Re: [PATCH 7/8] Validate symbol file using build-id.

Jan Kratochvil
2013-04-17T20:54:22Z

On Tue, 16 Apr 2013 17:15:00 +0200, Aleksandar Ristovski wrote:
> On 13-04-14 10:17 AM, Jan Kratochvil wrote:
> >On Tue, 09 Apr 2013 17:27:44 +0200, Aleksandar Ristovski wrote:
> >[...]
> >>--- a/gdb/solib-svr4.c
> >>+++ b/gdb/solib-svr4.c
> >[...]
[...]
> >>+ if (target_read_memory (bfd_get_section_vma (so->abfd, asec)
> >>+ + lm_addr_check (so, so->abfd),
> >
> >I see it is the most easy one how to get the target VMA of the options we were
> >discussing.
>
> I do not parse this unambiguously... did you mean this is the
> easiest way of getting target VMA? If so, I agree.

Yes.

> >>+ {
> >>+ build_idsz
> >>+ = extract_unsigned_integer ((gdb_byte *) note->descsz,
> >>+ sizeof (note->descsz),
> >>+ byte_order);
> >>+
> >>+ if (build_idsz == elf_tdata (so->abfd)->build_id->size)
> >>+ {
> >>+ const char gnu[4] = "GNU\0";
> >>+
> >>+ if (memcmp (note->name, gnu, 4) == 0)
> >>+ {
> >>+ ULONGEST namesz
> >>+ = extract_unsigned_integer ((gdb_byte *) note->namesz,
> >>+ sizeof (note->namesz),
> >>+ byte_order);
> >>+ CORE_ADDR build_id_offs;
> >>+
> >>+ /* Rounded to next sizeof (ElfXX_Word). */
> >>+ namesz = ((namesz + (sizeof (note->namesz) - 1))
> >>+ & ~((ULONGEST) (sizeof (note->namesz) - 1)));
> >
> >Like in the previous patch the rounding should be 4 bytes according to the
> >standard. Not a sizeof of anything.
>
> Yes, I re-read it, you are right: it is 4 byte aligned. Somehow I
> remembered it as Word aligned.

But I do not see it fixed (to use literal constant 4) in solib-svr4.c.

(Checking your GIT branch, not your posted patch, I hope they match.)

> >>+ warning (_("Shared object \"%s\" could not be validated "
> >>+ "and will be ignored."), so->so_name);
> >
> >Maybe the warning should by printed by svr4_validate stating the build-id does
> >not match (even possibly printing the both build-ids). And then sure one can
> >remove the warning here.
>
> I was thinking about that, but then decided that svr4_validate
> should simply provide an answer, not print warnings. User of the
> function should warn if appropriate (as is done in the patch).

The problem is what end-user will do with a situation GDB prints
Shared object \"%s\" could not be validated and will be ignored.

and the symbols are missing? The user has to start investigating target and
local shared library, it would be easier if GDB already printed both local and
remote build-ids which it already knows. It may be easier for user to check
it then.

Or why don't you find useful to print both build-ids?

Not a blocker, one can easily change the message in the future depending on
how it will be useful in practice.

> This is along the lines of (see below) using build-id for e.g.
> finding separate debug info etc...

When there are problems finding separate debug info it is already difficult to
troubleshoot, I commonly use strace on gdb, that could be also improved (that
is sure very unrelated to your current patchset).

> >>@@ -551,6 +562,7 @@ free_so (struct so_list *so)
> >> {
> >> struct target_so_ops *ops = solib_ops (target_gdbarch ());
> >>
> >>+ xfree (so->build_id);
> >> free_so_symbols (so);
> >> ops->free_so (so);
> >>
> >
> >I already wrote:
> >
> >The problem is there is:
> > xfree (so->build_id);
> >in free_so() but it should be in free_so_symbols instead. free_so_symbols is
> >called also from reload_shared_libraries_1 where so_list->abfd can change.
> >Then obviously one should also set so->build_id = NULL there.
>
>
> I'm not sure I follow this. In cases where so->build_id is
> determined it is good for the lifetime of the 'so', not symbols. It
> can not change for the given 'so'.

OK, I take my comment back. You are right current svr4_validate() never
stores the linux-nat-mode fetched build-id into so_list. so_list->build_id
contains only build-id from gdbserver which is always right.

Therefore I agree xfree in free_so is OK.

> >I was thinking making the BUILD_ID data private so solib-svr4.c but that is
> >currently not possible, lm_info is private for solib-svr4.c but that has
> >lifetime of so_list, not the lifetime of so_list->abfd.
>
>
> I was thinking slightly differently: build-id should be generalized
> to represent "an ID". For svr4, it is build-id, for some other
> system it might be something else, but if present, should represent
> the connection between target binary, binary at hand and associated
> optional separate debug info.

Not comment "what if" but currently there is no solib-backend-specific subpart
of so_list which is not trashed during reread_symbols (like so_list->lm_info
is) so there isn't where else to place the gdbserver build_id field now.

Thanks,
Jan