Rewrite the script to add gdb index to binary files

Rewrite script to add gdb index to binary files

This is a clean rewrite of ajwong@'s add-gdb-index script.

BUG=NONE


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149791

[vrk (LEFT CHROMIUM), 2012-08-01 23:44:52]

Here it is! (And this CL doesn't remove gdb-add-index because I didn't want to
look at it whatsoever :P)

[vrk (LEFT CHROMIUM), 2012-08-01 23:49:44]

Sending review to fischman@ instead since ajwong is tainted!

[awong, 2012-08-02 00:11:00]

On 2012/08/01 23:49:44, Victoria Kirst wrote:
> Sending review to fischman@ instead since ajwong is tainted!

For the record, I don't really think there's any actual taint, but it's just
safer if I keep completely out of this cause it's not worth the time to argue
about it. :-/

[Ami GONE FROM CHROMIUM, 2012-08-02 18:16:52]

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh
File build/add-index.sh (right):

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:27: local so_files=$(ldd $1 2>/dev/null \
This will recursively process the .so's that the .so's depend on, probably
incorrectly.
I think you want to do this ldd business outside this function and then call
this function in a loop (and remove the recursive call below).

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:37: function rm_directory {
s/rm_/rm_tmp_/

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:43: trap rm_directory EXIT
Only register this after the mktemp -d?

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:51: if [[ ! -f $FILENAME ]]; then
""-quote $FILENAME everywhere to avoid spaces changing syntax.

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:56: # Check to make sure ldd exists.
Why check for ldd's existence but not gdb's?
Are you planning to check for gdb version number (to make sure it supports the
index functionality you use)?

[vrk (LEFT CHROMIUM), 2012-08-02 22:51:32]

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh
File build/add-index.sh (right):

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:27: local so_files=$(ldd $1 2>/dev/null \
On 2012/08/02 18:16:52, Ami Fischman wrote:
> This will recursively process the .so's that the .so's depend on, probably
> incorrectly.
> I think you want to do this ldd business outside this function and then call
> this function in a loop (and remove the recursive call below).

Done.

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:37: function rm_directory {
On 2012/08/02 18:16:52, Ami Fischman wrote:
> s/rm_/rm_tmp_/

Inlined instead.

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:43: trap rm_directory EXIT
On 2012/08/02 18:16:52, Ami Fischman wrote:
> Only register this after the mktemp -d?

Done.

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:51: if [[ ! -f $FILENAME ]]; then
On 2012/08/02 18:16:52, Ami Fischman wrote:
> ""-quote $FILENAME everywhere to avoid spaces changing syntax.

Done.

https://chromiumcodereview.appspot.com/10843037/diff/1002/build/add-index.sh#...
build/add-index.sh:56: # Check to make sure ldd exists.
On 2012/08/02 18:16:52, Ami Fischman wrote:
> Why check for ldd's existence but not gdb's?
Ah, I just thought gdb was more likely to already exist. Check added.

> Are you planning to check for gdb version number (to make sure it supports the
> index functionality you use)?
I am now! Done... though not very elegantly.

[Ami GONE FROM CHROMIUM, 2012-08-02 23:01:15]

LGTM

https://chromiumcodereview.appspot.com/10843037/diff/9001/build/add-index.sh
File build/add-index.sh (right):

https://chromiumcodereview.appspot.com/10843037/diff/9001/build/add-index.sh#...
build/add-index.sh:25: exit
each of the error exits here and below should exit 1 to signal failure.

https://chromiumcodereview.appspot.com/10843037/diff/9001/build/add-index.sh#...
build/add-index.sh:41: exit
You're checking ldd & gdb, but assuming readelf and objcopy.
TBH I would just drop all these checks, letting set -e above catch everything.

[vrk (LEFT CHROMIUM), 2012-08-02 23:24:55]

Thanks for reviewing, Ami!

https://chromiumcodereview.appspot.com/10843037/diff/9001/build/add-index.sh
File build/add-index.sh (right):

https://chromiumcodereview.appspot.com/10843037/diff/9001/build/add-index.sh#...
build/add-index.sh:25: exit
On 2012/08/02 23:01:15, Ami Fischman wrote:
> each of the error exits here and below should exit 1 to signal failure.

Done.

https://chromiumcodereview.appspot.com/10843037/diff/9001/build/add-index.sh#...
build/add-index.sh:41: exit
On 2012/08/02 23:01:15, Ami Fischman wrote:
> You're checking ldd & gdb, but assuming readelf and objcopy.
> TBH I would just drop all these checks, letting set -e above catch everything.

Done.

[commit-bot: I haz the power, 2012-08-03 00:15:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/10843037/4002

[commit-bot: I haz the power, 2012-08-03 02:56:37]

Change committed as 149791