Re: [PATCH] Read corrrect auxiliary entry in AIX.

["Sangamesh Mallayya" <sangamesh dot swamy at in dot ibm dot com>]

"Ulrich Weigand" <uweigand@de.ibm.com> wrote on 11/22/2016 11:38:20 PM:

> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> To: Sangamesh Mallayya/India/IBM@IBMIN
> Cc: gdb-patches@sourceware.org, Ulrich.Weigand@de.ibm.com (Ulrich 
Weigand)
> Date: 11/22/2016 11:38 PM
> Subject: Re: [PATCH] Read corrrect auxiliary entry in AIX.
> 
> Sangamesh Mallayya wrote:
> 
> > Thanks for review and sorry for the long delay in responding to your 
> > comments.
> 
> No problem, thanks for the details, which seem to confirm what I had
> been assuming.
> 
> > I checked the dbx behaviour in case of C_WEAKEXT.
> > It seems to be handling C_WEAKEXT symbols too.
> > So i think we might need to think of handling C_WEAKEXT too.
> > But one question comes to mind is, why this case wasn't consider at 
the 
> >first place itself.
> 
> Well, I wouldn't be surprised if this code in GDB was older than support
> for weak symbols in COFF, and was never updated when those were added 
...
> 
> > > - Given the check above, can it *ever* happen that this check is 
false
> > >   and we still reach the LD/PR case?  If so, we'd still have the 
problem
> > >   that the record is the wrong one and doesn't actually contain a 
size,
> > >   right?  But if not, shouldn't we just have the 
function_entry_point
> > >   handling in one place and avoid the goto?
> > > 
> > > - If there is just one place, should it be where you added the check
> > >   above, or rather in the LD/PR switch case?
> > > 
> > > - Finally, given the check added above, can it still ever happen 
that
> > >   the original ISFCN check after the if C_EXT block is true?  If so,
> > >   are we sure that the symbol will always have a function size in 
the
> > >   first aux entry?  (I understand that if a symbol has just one aux
> > >   entry, it must be a csect one which doesn't have the function 
size.)
> > >   If not, this block should be removed as well.
> > > 
> > 
> > Thanks for catching this.
> > 
> > With current modification looks like we mayn't come to the original 
ISFCN 
> > check, as we aren't restricted on only one auxiliary entry
> > and ISFCN check result to true for the functions with two auxiliary 
entry.
> > 
> > Yes. As mentioned below the order of auxiliary entry is important and 
for 
> > functions first auxiliary entry is having the size information.
> > 
> > Let me know your view. 
> 
> So then I would suggest to restructure the logic along those lines:
> 
>       if (cs->c_sclass == C_EXT || cs->c_sclass == C_HIDEXT
>           || cs->c_sclass == C_WEAKEXT)
>    {
>      if (cs->c_naux > 1 && ISFCN (cs->c_type))
>        {
>          /* A function entry point.  */
>          fcn_start_addr = cs->c_value;
> 
>          /* Save the function header info, which will be used
>        when `.bf' is seen.  */
>          fcn_cs_saved = *cs;
> 
>          /* Read in main aux header to get the function size.  */
>          bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type,
>                 cs->c_sclass, 0, cs->c_naux,
>                 &fcn_aux_saved);
>          continue;
>             }
> 
> 
>           /* Otherwise, read the CSECT aux header, which is always
>              the last one by convention.  */
>      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);
> 
>           switch (CSECT_SMTYP (&main_aux))
> 
> .... same code as currently, but remove the handling of LD/PR ...
> 
>         }
> 
> ... remove the if (ISFCN ...) block ...
> 
> 
> Does this make sense to you?
> 

Thanks for the suggested patch and i totally agree.
Pasted the changes below i have it right now. Perhaps some comments might 
need some more editing.
Didn't see any new regression failures with this change.

I will check this with the master branch and run the tests again.
Let me know your view on this. 

@@ -1139,9 +1139,9 @@
          cur_src_end_addr = first_object_file_end;
          /* 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 ||
+         cs->c_sclass == C_WEAKEXT)
        {
          /* Dealing with a symbol with a csect entry.  */
 
@@ -1151,9 +1151,40 @@
 #define        CSECT_SMTYP(PP) (SMTYP_SMTYP(CSECT(PP).x_smtyp))
 #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);
+        /* Convert the auxent to something we can access.
+            XCOFF can have more than auxiliary entries.
+            Actual functions will have two auxiliary entries.
+            One to have the function size and other to have the 
smtype/smclass (LD/PR)
+            information.
+            c_type value of main symbol table is set only in case of 
C_EXT/C_HIDEEXT/C_WEAKEXT.
+            bit 10 of type is set if symbol is a function, ie the value 
is set to 32(0x20).
+            So read the first function auxiliay entry which contains the 
size. */
+          if (cs->c_naux > 1 && ISFCN (cs->c_type))
+          {
+              /* a function entry point.  */
+
+              fcn_start_addr = cs->c_value;
+
+              /* save the function header info, which will be used
+                 when `.bf' is seen.  */
+              fcn_cs_saved = *cs; 
+
+            /* 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, &fcn_aux_saved);
+              continue;
+          }
+          /* Read the csect auxiliary header, which is always the last by 

+             convention. */
+          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);
+ 
 
          switch (CSECT_SMTYP (&main_aux))
            {
@@ -1238,17 +1269,6 @@
 
              switch (CSECT_SCLAS (&main_aux))
                {
-               case XMC_PR:
-                 /* a function entry point.  */
-               function_entry_point:
-
-                 fcn_start_addr = cs->c_value;
-
-                 /* save the function header info, which will be used
-                    when `.bf' is seen.  */
-                 fcn_cs_saved = *cs;
-                 fcn_aux_saved = main_aux;
-                 continue;
 
                case XMC_GL:
                  /* shared library function trampoline code entry point. 
*/
@@ -1280,16 +1300,6 @@
            }
        }
 
-      /* If explicitly specified as a function, treat is as one.  This 
check
-        evaluates to true for @FIX* bigtoc CSECT symbols, so it must 
occur
-        after the above CSECT check.  */
-      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;
-       }
-
       switch (cs->c_sclass)
        {
        case C_FILE:
@@ -1571,7 +1581,7 @@
       SYMBOL_ACLASS_INDEX (sym) = LOC_BLOCK;
       SYMBOL_DUP (sym, sym2);
 
-      if (cs->c_sclass == C_EXT)
+      if (cs->c_sclass == C_EXT || C_WEAKEXT)
        add_symbol_to_list (sym2, &global_symbols);
       else if (cs->c_sclass == C_HIDEXT || cs->c_sclass == C_STAT)
        add_symbol_to_list (sym2, &file_symbols);
@@ -2247,6 +2257,7 @@
        {
        case C_EXT:
        case C_HIDEXT:
+       case C_WEAKEXT:
          {
            /* The CSECT auxent--always the last auxent.  */
            union internal_auxent csect_aux;