This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Read correct xcoff auxilliary entry
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Sangamesh Mallayya <sangamesh dot swamy at in dot ibm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sun, 6 Dec 2015 13:45:07 +0100
- Subject: Re: [PATCH] Read correct xcoff auxilliary entry
- Authentication-results: sourceware.org; auth=none
- References: <OFA2A5A796 dot D5CF123B-ON65257EF1 dot 0061CF22-65257EF1 dot 0062BB63 at LocalDomain> <OFE806554B dot 07173E08-ON65257EF8 dot 0025C68E-65257EF8 dot 0025D8F5 at LocalDomain> <201511170617 dot tAH6Hmsg021436 at d28av01 dot in dot ibm dot com>
Sangamesh,
Very, very, VERY, sorry about the late review. Please feel free to
ping a couple of weeks after first submission, and then weekly after
that.
> Attached patch resolves the below errors in case of debugging XCOFF in
> AIX.
>
> => warning: (Internal error: pc 0x10000380 in read in psymtab, but not in
> symtab.)
>
> => Read the correct filename if the binary is compiled with -qfuncsect
> option.
>
> (gdb) br main
> Breakpoint 1 at 0x10000394: file __start__, line 24.
>
> And also has the fix to skip reading symbols starting with @Fix.
> Symbols starting with '@FIX' are used for TOC reference and need to be
> skipped.
>
> Make name of current file as pst->filename instead of _start_ as this does
> not handle the case when there are multiple auxillary entries,
> eg: when compiled with 'xlc -qfuncsect' option.
>
> We also need to read the correct auxillary entry.
> i.e
>
> bfd_coff_swap_aux_in (abfd,
> raw_auxptr + ((coff_data
> (abfd)->local_symesz) * (cs->c_naux - 1)),
> cs->c_type, cs->c_sclass, cs->c_naux
> - 1,
> cs->c_naux, &main_aux);
>
>
My first comment is that you are combining multiple fixes into a single
patch, so you first need to split it into individual patches. In case
you haven't seen it yet, we have a contribution checklist at:
https://sourceware.org/gdb/wiki/ContributionChecklist
It'll tell you how we expect the patches to be submitted so we can
easily include them. For your patches, it would be good for each patch
show what the incorrect behavior is (Eg. copy/paste of a GDB debugging
session, similar to what you did with the "br main" example), explain
what you expected, and then explain what was wrong and how you fixed it.
This is going to be particularly important in the section where you add
support for multiple-auxent entries.
> --- ./gdb/xcoffread.c_orig 2015-11-02 05:33:01.000000000 -0600
> +++ ./gdb/xcoffread.c 2015-11-02 05:29:56.000000000 -0600
> @@ -1035,7 +1035,7 @@
> union internal_auxent fcn_aux_saved = main_aux;
> struct context_stack *new;
>
> - char *filestring = " _start_ "; /* Name of the current file. */
> + char *filestring = pst -> filename; /* Name of the current file. */
No spaces around the -> operator. Thus:
char *filestring = pst->filename; /* Name of the current file. */
> /* if symbol name starts with ".$" or "$", ignore it. */
> - if (cs->c_name[0] == '$'
> + /* We also need to skip symbols starting with @FIX, which are used for TOC reference */
> + if (cs->c_name[0] == '$' || !strncmp(cs->c_name, "@FIX", 4)
> || (cs->c_name[1] == '$' && cs->c_name[0] == '.'))
Comments should be full sentences, starting with a capital letter
(good), and ending with a period (missing). Periods are always followed
by a double space. Also, it seems a little odd to have two comments
in a row. Why not do:
/* if symbol name starts with ".$" or "$", ignore it.
We also need to skip symbols starting with @FIX, which are used
for TOC references. */
(I think there should be an 's' at the end of "reference", btw).
And finally, I think it would be more readable if you moved the extra
"|| !strncmp(cs->c_name, "@FIX", 4)" at the start of the next line,
so that, visually, we see 3 alternatives that could make the condition
be true.
Note that we now have a function called startswith that you should use.
> continue;
>
> @@ -1148,8 +1149,7 @@
> /* Done with all files, everything from here on is globals. */
> }
>
> - if ((cs->c_sclass == C_EXT || cs->c_sclass == C_HIDEXT)
> - && cs->c_naux == 1)
> + if ((cs->c_sclass == C_EXT || cs->c_sclass == C_HIDEXT))
> {
> /* Dealing with a symbol with a csect entry. */
>
> @@ -1160,8 +1160,27 @@
> #define CSECT_SCLAS(PP) (CSECT(PP).x_smclas)
>
> /* Convert the auxent to something we can access. */
> - bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type, cs->c_sclass,
> - 0, cs->c_naux, &main_aux);
> + /* xcoff can have more than 1 auxent */
See above about comments being sentences... By the way, the proper
spelling for xcoff is XCOFF, it seems. I'm sure we got it wrong
elsewhere, but let's try to make the new ones be consistent with
the official documentation...
Can you also augment the comment to explain when that can happen and
what it means. For the reader not quite familiar with the XCOFF format,
these are extremely useful in understand the code faster, and also
knowing where to look if more info is needed.
> + if (cs->c_naux > 1)
> + {
> + if (ISFCN (cs->c_type) && cs->c_sclass != C_TPDEF)
> + {
> + bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type,
> + cs->c_sclass, 0, cs->c_naux,
> + &main_aux);
> + goto function_entry_point;
> + }
> + else
> + bfd_coff_swap_aux_in (abfd,
> + raw_auxptr + ((coff_data (abfd)->local_symesz) * (cs->c_naux - 1)),
> + cs->c_type, cs->c_sclass, cs->c_naux - 1,
> + cs->c_naux, &main_aux);
Please reformat the above to avoid exceeding 80 characters per line.
I am thinking:
bfd_coff_swap_aux_in
(abfd,
raw_auxptr
+ ((coff_data (abfd)->local_symesz) * (cs->c_naux - 1)),
cs->c_type, cs->c_sclass, cs->c_naux - 1,
cs->c_naux, &main_aux);
> + }
> + else if (cs->c_naux == 1)
> + bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type, cs->c_sclass,
> + 0, cs->c_naux, &main_aux);
Same (line too long)
> + else
> + continue ;
No space before ';' -> "continue;".
Note that I haven't reviewed the meat of that last hunk yet, only
the trivialities such as formatting, because I don't really get
from your description what is really going on. Is it related to
the warning about a symbol being in psymtab but on in symtab?
I'll review more carefully once you split the changes and send
a more detailed description for each patch.
Thank you,
--
Joel