find-debuginfo.sh change for gdb index

[Tom Tromey]

Hi Panu. Roland suggested I contact you as the Fedora RPM maintainer.

This patch adds gdb index creation to find-debuginfo.sh, in support of a
feature I'm proposing for Fedora 14:

https://fedoraproject.org/wiki/Features/GdbIndex

I've tested this just by building some RPMs locally.

If you'd prefer, I can open a bug in bugzilla for this.

I don't know what other changes may be needed to ensure that the proper
gdb is in the buildroots when this script is run. Also, the proper gdb
is not actually available yet; cleaning up that patch series is my next
task.

I think this should probably be local to Fedora, but if you think it
should go into upstream RPM, I am happy to try that.

Tom

--- find-debuginfo.sh.orig 2010-06-29 16:19:42.000000000 -0600
+++ find-debuginfo.sh 2010-06-29 15:52:08.000000000 -0600
@@ -96,6 +96,15 @@
chmod 444 "$1" || exit
}

+# Create a gdb .index file for $1.
+make_gdb_index()
+{
+ local f="$1"
+ local d="${f%/*}"
+ # We don't care if gdb gives an error.
+ gdb --batch-silent -ex "file $f" -ex "maintenance save-gnu-index $d" > /dev/null 2>&1
+}
+
# Make a relative symlink to $1 called $3$2
shopt -s extglob
link_relative()
@@ -224,9 +233,15 @@
chmod u-w "$f"
fi

+ make_gdb_index "$debugfn"
+
if [ -n "$id" ]; then
make_id_link "$id" "$dn/$(basename $f)"
make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug
+
+ if [ -f "${debugfn}.index" ]; then
+ make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug
+ fi
fi
done || exit

[Roland McGrath]

Post by Tom Tromey
I don't know what other changes may be needed to ensure that the proper
gdb is in the buildroots when this script is run. Also, the proper gdb
is not actually available yet; cleaning up that patch series is my next
task.

The rpm-build subpackage will need "Requires: gdb >= V-R". Obviously, the
patch can't go in anywhere until that gdb is built in dist-rawhide.

Post by Tom Tromey
I think this should probably be local to Fedora, but if you think it
should go into upstream RPM, I am happy to try that.

It's certainly Fedora-specific and only for Fedora 14.

Post by Tom Tromey
+ gdb --batch-silent -ex "file $f" -ex "maintenance save-gnu-index $d" > /dev/null 2>&1

I don't quite understand what file this writes to.
Is it implicitly "<symfile name>.index" in the argument directory?

IMHO, the file name should have "gdb" in the name.
This is really not any very generic sort of index for the information.

Post by Tom Tromey
if [ -n "$id" ]; then
make_id_link "$id" "$dn/$(basename $f)"
make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug
+
+ if [ -f "${debugfn}.index" ]; then
+ make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug
+ fi

What's this for? It just repeats the work of making and recording the
build-id symlink to the .debug file. Unless you're being quite subtle
somehow I've missed, this doesn't do anything with the index file.
Do you mean something like:

make_id_link "$id" "/usr/lib/debug$dn/$bn" .index

? That gets you a /usr/lib/debug/.build-id/xx/yyy.index symlink
to ../../usr/bin/foobar.index for example.


Thanks,
Roland

[Tom Tromey]

Roland> I don't quite understand what file this writes to.
Roland> Is it implicitly "<symfile name>.index" in the argument directory?

Yes.

Roland> IMHO, the file name should have "gdb" in the name.
Roland> This is really not any very generic sort of index for the information.

Ok. What do you think of just ".gdb as the suffix?

Post by Tom Tromey
+ if [ -f "${debugfn}.index" ]; then
+ make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug
+ fi

Roland> What's this for? It just repeats the work of making and recording the
Roland> build-id symlink to the .debug file. Unless you're being quite subtle
Roland> somehow I've missed, this doesn't do anything with the index file.
Roland> Do you mean something like:

Roland> make_id_link "$id" "/usr/lib/debug$dn/$bn" .index

Roland> ? That gets you a /usr/lib/debug/.build-id/xx/yyy.index symlink
Roland> to ../../usr/bin/foobar.index for example.

Yeah, oops. I will fix this.

Tom

[Roland McGrath]

Post by Tom Tromey
Roland> I don't quite understand what file this writes to.
Roland> Is it implicitly "<symfile name>.index" in the argument directory?
Yes.

Ok. IMHO it would be better if the command just took the output file name
explicitly. But whatever.

Post by Tom Tromey
Roland> IMHO, the file name should have "gdb" in the name.
Roland> This is really not any very generic sort of index for the information.
Ok. What do you think of just ".gdb as the suffix?

I don't object, though I'd go with something like '.gdb-index' instead.


Thanks,
Roland

[Tom Tromey]

Roland> I don't quite understand what file this writes to.
Roland> Is it implicitly "<symfile name>.index" in the argument directory?

Yes.

Roland> Ok. IMHO it would be better if the command just took the output
Roland> file name explicitly. But whatever.

It saves indices for all the loaded objfiles.
I suppose I could add another argument so you can specify which one.

Roland> I don't object, though I'd go with something like '.gdb-index' instead.

Ok, I'll do that.

Tom

[Tom Tromey]

Tom> It saves indices for all the loaded objfiles.
Tom> I suppose I could add another argument so you can specify which one.

I left this as-is.

I renamed the command, though, since a different name made more sense to
me once I wrote the gdb documentation.

Here's a new find-debuginfo.sh patch.

Tom

--- find-debuginfo.sh.orig 2010-06-29 16:19:42.000000000 -0600
+++ find-debuginfo.sh 2010-06-30 14:41:19.000000000 -0600
@@ -96,6 +96,15 @@
chmod 444 "$1" || exit
}

+# Create a .gdb-index file for $1.
+make_gdb_index()
+{
+ local f="$1"
+ local d="${f%/*}"
+ # We don't care if gdb gives an error.
+ gdb --batch-silent -ex "file $f" -ex "maintenance save-gdb-index $d" > /dev/null 2>&1
+}
+
# Make a relative symlink to $1 called $3$2
shopt -s extglob
link_relative()
@@ -224,6 +233,8 @@
chmod u-w "$f"
fi

+ make_gdb_index "$debugfn"
+
if [ -n "$id" ]; then
make_id_link "$id" "$dn/$(basename $f)"
make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug

[Roland McGrath]

Post by Tom Tromey
Here's a new find-debuginfo.sh patch.

So you don't need a build-id symlink for the index after all?
Where does gdb look for an index file?

[Tom Tromey]

Tom> Here's a new find-debuginfo.sh patch.

Roland> So you don't need a build-id symlink for the index after all?

I thought not, but I am doing a much fuller check now.

Roland> Where does gdb look for an index file?

For a symbol file X, it looks for X.gdb-index.

Tom

[Tom Tromey]

Roland> So you don't need a build-id symlink for the index after all?

Tom> I thought not, but I am doing a much fuller check now.

I cleaned out all my old index files from /usr/lib/debug.
Then I re-built the index files -- but not for .debug files in
/usr/lib/debug/.build-id.

I think something in gdb must be realpath-ing the .build-id links.
A casual search didn't turn it up, though.

It may still be prudent to make the links, I don't know.
Maybe Jan has more insight here.

Tom

[Tom Tromey]

Tom> I cleaned out all my old index files from /usr/lib/debug.
Tom> Then I re-built the index files -- but not for .debug files in
Tom> /usr/lib/debug/.build-id.

I forgot to say: then I attached to a running OO.o, and used "maint
print objfiles" to see whether the index was in use for each objfile.
It was.

Tom

[Roland McGrath]

Post by Tom Tromey
For a symbol file X, it looks for X.gdb-index.

It's not clear to me what that means in the separate .debug case.
If "symbol file" means the file with the DWARF, then that file is
foo.debug so you will be looking for foo.debug.gdb-index.

A good way to deal with the build-id symlink is to canonicalize_file_name
(aka realpath) them and call the symlink target the real file name (for
user display of a useful file name too). If the gdb build-id support is
doing that, then foo.debug.gdb-index should be found.

I think it is preferable for the packaging not to add the new symlinks.
It doesn't seem like we really need them, since you can look at the
.build-id/xx/yyy.debug symlink target name instead.
It just adds a lot more crapola to each rpm.


Thanks,
Roland

[Tom Tromey]

Tom> For a symbol file X, it looks for X.gdb-index.

Roland> It's not clear to me what that means in the separate .debug case.
Roland> If "symbol file" means the file with the DWARF, then that file is
Roland> foo.debug so you will be looking for foo.debug.gdb-index.

Right, that is what we do.

Roland> I think it is preferable for the packaging not to add the new symlinks.
Roland> It doesn't seem like we really need them, since you can look at the
Roland> .build-id/xx/yyy.debug symlink target name instead.

Sounds good.

In this case I think the most recent patch I sent is the one to use.

Tom

[Panu Matilainen]

Post by Tom Tromey
Tom> For a symbol file X, it looks for X.gdb-index.
Roland> It's not clear to me what that means in the separate .debug case.
Roland> If "symbol file" means the file with the DWARF, then that file is
Roland> foo.debug so you will be looking for foo.debug.gdb-index.
Right, that is what we do.
Roland> I think it is preferable for the packaging not to add the new symlinks.
Roland> It doesn't seem like we really need them, since you can look at the
Roland> .build-id/xx/yyy.debug symlink target name instead.
Sounds good.
In this case I think the most recent patch I sent is the one to use.

So it would be this patch, right?

--- find-debuginfo.sh.orig 2010-06-29 16:19:42.000000000 -0600
+++ find-debuginfo.sh 2010-06-30 14:41:19.000000000 -0600
@@ -96,6 +96,15 @@
chmod 444 "$1" || exit
}

+# Create a .gdb-index file for $1.
+make_gdb_index()
+{
+ local f="$1"
+ local d="${f%/*}"
+ # We don't care if gdb gives an error.
+ gdb --batch-silent -ex "file $f" -ex "maintenance save-gdb-index $d" > /dev/null 2>&1
+}
+
# Make a relative symlink to $1 called $3$2
shopt -s extglob
link_relative()
@@ -224,6 +233,8 @@
chmod u-w "$f"
fi

+ make_gdb_index "$debugfn"
+
if [ -n "$id" ]; then
make_id_link "$id" "$dn/$(basename $f)"
make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug

Is the necessary patch(es) already in rawhide gdb, I dont see anything
obviously related in gdb changelogs?

One thing this does is that it forces rpm-build to depend on gdb, which
hardly is the end of the world, just something to note.

- Panu -

[Jan Kratochvil]

Post by Panu Matilainen
Is the necessary patch(es) already in rawhide gdb, I dont see
anything obviously related in gdb changelogs?

It will be before the FeatureFreeze. I will ping you to coordinate with rpm.
So far it is only posted upstream:
http://sourceware.org/ml/gdb-patches/2010-06/msg00696.html


Thanks,
Jan

[Panu Matilainen]

Post by Jan Kratochvil

Post by Panu Matilainen
Is the necessary patch(es) already in rawhide gdb, I dont see
anything obviously related in gdb changelogs?

It will be before the FeatureFreeze. I will ping you to coordinate with rpm.
http://sourceware.org/ml/gdb-patches/2010-06/msg00696.html

Well, I'm starting a long vacation at the end of this week so you might
need to ping Jindrich Novy and/or Florian Festi instead.

OTOH if we can reasonably expect the details wrt find-debuginfo to stay
unchanged we can just apply the patch already (AFAICT it ignores errors
anyway and wont cause build-failures)

- Panu -

[Tom Tromey]

Panu> So it would be this patch, right?

Yes.

Panu> Is the necessary patch(es) already in rawhide gdb, I dont see anything
Panu> obviously related in gdb changelogs?

Yeah, it is still under discussion upstream.

One thing that came up is that the current approach of using file
size+mtime to determine whether the index is valid is not super.
Two competing ideas:

1. Put the build-id into the index file, then verify it.
2. Require the index to be a section in the ELF object.
This has the nice property that no validation need be done.
However, it would require a further tweak to find-debuginfo.sh.

Any comments?
I would like to resolve this ASAP.

Tom

[Roland McGrath]

Post by Tom Tromey
One thing that came up is that the current approach of using file
size+mtime to determine whether the index is valid is not super.

Agreed.

Post by Tom Tromey
1. Put the build-id into the index file, then verify it.

That is the method already used for .debug files today. (The old
.gnu_debuglink CRC32 check should be ignored when there are build IDs.
I don't know about GDB, but libdw ignores it when there is a build ID.)

Post by Tom Tromey
2. Require the index to be a section in the ELF object.

This is the most obvious way to handle this information. The only real
down-sides are a) possible bugs breaking the original file and b) somewhat
more hair later if the index format has to be redone and only data
reindexed. I don't really think either of those is something to worry much
about.

Doing this adds another interlocking requirement. The eu-strip (libebl)
code for -g matches the individual DWARF sections by name. (Without -g, it
strips all non-allocated sections, but for -g it only strips the sections
with recognized names.) So that requires a (one-line) change in elfutils
for the new section name, and we'll need the new elfutils release in
buildroots before the new index-adding procedure goes in. (It shouldn't be
any problem to push this out quickly, but it needs to go on your checklist
so we coordinate it.)

Post by Tom Tromey
This has the nice property that no validation need be done.

Put another way, it implicitly takes advantage of the existing validation
mechanisms. If you modify the unstripped file before strip-to-file, then
even the CRC32 will be correct. (This should not really be a concern one
way or the other for Fedora, where both --build-id is always used and all
the tools should be validating by IDs rather than CRCs. But for generic
goodness of the scheme, it is attractive in not introducing a new wrinkle
about the CRC calculation.)

Post by Tom Tromey
However, it would require a further tweak to find-debuginfo.sh.

Or perhaps less change there, after a fashion. The place to put the new
work is either right after debugedit or right before eu-strip -f. Off hand
I think it should be the former. That will put an index into e.g. the
unstripped vmlinux, which gets copied into /usr/lib/debug but never passed
to eu-strip.

So one approach would be to replace the debugedit invocation with the use
of another shell script. Then that new script can run debugedit and make
the gdb index. It can be fiddled as needed without changing the core logic
in find-debuginfo.sh again. We could possibly let that script be supplied
by the redhat-rpm-config rpm or the gdb rpm or something else, so that its
maintenance is not directly in the path of the rpm package maintainers.


Thanks,
Roland

[Tom Tromey]

Roland> Doing this adds another interlocking requirement. The eu-strip
Roland> (libebl) code for -g matches the individual DWARF sections by
Roland> name. (Without -g, it strips all non-allocated sections, but
Roland> for -g it only strips the sections with recognized names.) So
Roland> that requires a (one-line) change in elfutils for the new
Roland> section name, and we'll need the new elfutils release in
Roland> buildroots before the new index-adding procedure goes in. (It
Roland> shouldn't be any problem to push this out quickly, but it needs
Roland> to go on your checklist so we coordinate it.)

I was thinking we would just objcopy the data into the .debug file in
find-debuginfo.sh.

Is there an advantage to doing it before the stripping?

Tom

[Roland McGrath]

Post by Tom Tromey
I was thinking we would just objcopy the data into the .debug file in
find-debuginfo.sh.
Is there an advantage to doing it before the stripping?

I stated two:

1. CRC32 in .gnu_debuglink will be correct.

2. Do it to those that don't get stripped.
(AFAIK the only case is vmlinux.)

I am not especially concerned about either of those issues. But they point
to it probably being the more right or more clean approach.

I am far more concerned about the unknown potential problems.
For example, making sure that eu-unstrip can still recombine the files.
(I think that will be fine, but it's something to worry about.)

In the past we have had various problems with objcopy/bfd deciding to
gratuitously regenerate things and breaking the data along the way.
I believe all the old issues we ever noticed have been fixed by now.
But it certainly makes me nervous. I trust eu-strip far more.


Thanks,
Roland

[Tom Tromey]

Roland> So one approach would be to replace the debugedit invocation
Roland> with the use of another shell script.

Here is a patch to just do the work directly in find-debuginfo.sh. This
seemed simpler to me, but if you and Panu want a new script, I will do
that.

It would perhaps have been cleaner to make the gdb command simply
rewrite the objfile directly. However, this turns out to be relatively
hairy with BFD. So again, for simplicity I just stuck with invoking
objcopy directly. I also verified that objcopy will preserve hard
links.

Tom

--- find-debuginfo.sh.orig 2010-06-29 16:19:42.000000000 -0600
+++ find-debuginfo.sh 2010-07-08 09:36:03.000000000 -0600
@@ -96,6 +96,15 @@
chmod 444 "$1" || exit
}

+# Create a .gdb-index file for $1.
+make_gdb_index()
+{
+ local f="$1"
+ local d="${f%/*}"
+ # We don't care if gdb gives an error.
+ gdb --batch-silent -ex "file $f" -ex "maintenance save-gdb-index $d" > /dev/null 2>&1
+}
+
# Make a relative symlink to $1 called $3$2
shopt -s extglob
link_relative()
@@ -207,6 +216,12 @@
$strict && exit 2
fi

+ make_gdb_index "$f"
+ if [ -f "${f}.gdb-index" ]; then
+ objcopy --add-section .gdb_index="${f}.gdb-index" --set-section-flags .gdb_index=readonly "$f" "$f"
+ rm -f "${f}.gdb-index"
+ fi
+
# A binary already copied into /usr/lib/debug doesn't get stripped,
# just has its file names collected and adjusted.
case "$dn" in

[Tom Tromey]

Tom> Here is a patch to just do the work directly in find-debuginfo.sh. This
Tom> seemed simpler to me, but if you and Panu want a new script, I will do
Tom> that.

An upstream maintainer wanted the command's name changed.
It is now "save gdb-index" instead of "maint save-gdb-index".

Here's the updated find-debuginfo.sh patch.

Tom

--- find-debuginfo.sh.orig 2010-06-29 16:19:42.000000000 -0600
+++ find-debuginfo.sh 2010-07-08 14:10:06.000000000 -0600
@@ -96,6 +96,15 @@
chmod 444 "$1" || exit
}

+# Create a .gdb-index file for $1.
+make_gdb_index()
+{
+ local f="$1"
+ local d="${f%/*}"
+ # We don't care if gdb gives an error.
+ gdb --batch-silent -ex "file $f" -ex "save gdb-index $d" > /dev/null 2>&1
+}
+
# Make a relative symlink to $1 called $3$2
shopt -s extglob
link_relative()
@@ -207,6 +216,12 @@
$strict && exit 2
fi

+ make_gdb_index "$f"
+ if [ -f "${f}.gdb-index" ]; then
+ objcopy --add-section .gdb_index="${f}.gdb-index" --set-section-flags .gdb_index=readonly "$f" "$f"
+ rm -f "${f}.gdb-index"
+ fi
+
# A binary already copied into /usr/lib/debug doesn't get stripped,
# just has its file names collected and adjusted.
case "$dn" in

[Roland McGrath]

IMHO that procedure is now hairy enough that it really should be
encapsulated in some script provided by gdb (and maintained upstream if
the gdb support itself is upstream). Not to mention that all other
users of the index support would want such a script to use by hand,
integrate into a different package system, etc.

i.e., find-debuginfo.sh would just add:

gdb-add-index "$f" > /dev/null 2>&1


Thanks,
Roland

[Panu Matilainen]

Post by Roland McGrath
IMHO that procedure is now hairy enough that it really should be
encapsulated in some script provided by gdb (and maintained upstream if
the gdb support itself is upstream). Not to mention that all other
users of the index support would want such a script to use by hand,
integrate into a different package system, etc.
gdb-add-index "$f" > /dev/null 2>&1

Sounds good to me. Also if it were an external script that might
not be there (depending on gdb-version), find-debuginfo.sh can do

[ -x /usr/bin/gdb-add-index ] && /usr/bin/gdb-add-index "$f" \

Post by Roland McGrath
/dev/null 2>&1

making it nicer for upstreaming. Of course for F >= 14 we still want
rpm-build to require gdb to consistently get the indexes.

- Panu -

[Tom Tromey]

Post by Roland McGrath
gdb-add-index "$f" > /dev/null 2>&1

Panu> Sounds good to me. Also if it were an external script that might not
Panu> be there (depending on gdb-version), find-debuginfo.sh can do
[...]

Indeed, the latest patch is just the line you wrote.

I sent the gdb-add-index patch to gdb-patches. I don't know whether it
will go in (I think historically gdb has avoided installing scripts),
but in any case Jan is going to put it into the gdb RPM.

Tom

--- find-debuginfo.sh.orig 2010-06-29 16:19:42.000000000 -0600
+++ find-debuginfo.sh 2010-07-09 11:35:39.000000000 -0600
@@ -207,6 +207,8 @@
$strict && exit 2
fi

+ [ -x /usr/bin/gdb-add-index ] && /usr/bin/gdb-add-index "$f" > /dev/null 2>&1
+
# A binary already copied into /usr/lib/debug doesn't get stripped,
# just has its file names collected and adjusted.
case "$dn" in

[Tom Tromey]

Panu> Sounds good to me. Also if it were an external script that might not
Panu> be there (depending on gdb-version), find-debuginfo.sh can do
Tom> [...]

Tom> Indeed, the latest patch is just the line you wrote.

I just wanted to follow up on this, as the Fedora 14 process is moving
along rapidly. Did the needed RPM patch go in?

Tom

[Roland McGrath]

Post by Tom Tromey
I just wanted to follow up on this, as the Fedora 14 process is moving
along rapidly. Did the needed RPM patch go in?

Nope, it's not in the Fedora package nor in rpm's upstream git repository.

Note that we'll also need elfutils 0.149 to be in Fedora so that eu-strip
-g will recognize the .gdb_index section as one to move into .debug files.
(Otherwise they'll end up in libc.so and the like where -g is used, rather
than libc.so.debug, though most things don't use -g.)

I haven't released 0.149 yet, I guess I will next week.


Thanks,
Roland

[Jan Kratochvil]

Post by Tom Tromey
Panu> Sounds good to me. Also if it were an external script that might not
Panu> be there (depending on gdb-version), find-debuginfo.sh can do
Tom> [...]
Tom> Indeed, the latest patch is just the line you wrote.
I just wanted to follow up on this, as the Fedora 14 process is moving
along rapidly. Did the needed RPM patch go in?

It is filed for rpm and still open:
https://bugzilla.redhat.com/show_bug.cgi?id=617166


Regards,
Jan