Re: [PATCH] Read correct xcoff auxilliary entry

[Joel Brobecker <brobecker at adacore 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