Re: [PATCH,AIX] Enable libiberty to read AIX XCOFF

[DJ Delorie]

"REIX, Tony"  writes:
> It appears that XNEWVEC() calls xmalloc which prints a message and
> calls xexit if malloc fails.

Objection removed then ;-)

> So, yes, we check if (strtab == NULL) though there is no way that
> XDELETEVEC(NULL) breaks something.  However, it is a classic
> programming style.

Yup, I noted that.  Just mentioning the inconsistency.

Re: [PATCH,AIX] Enable libiberty to read AIX XCOFF

[David Edelsohn]

On Wed, Jun 7, 2017 at 10:22 AM, REIX, Tony  wrote:
> Hi David,
>
> I'll fix the code incorrectly indented.

I already have fixed the indentation in my copy.

>
> About your comment about our code looking for TEXT section by looking at 
> string ".text"  , please note that our patch fixes a file called 
> "simple-object-xcoff.c" : SIMPLE.
> Do not expect us to handle more than required.
>
> However, are you sure that  -ffunction-sections  is implemented on AIX ?

-ffunction-sections is implemented on AIX.  It is used quite
frequently for additional performance.  The libstdc++ library builds
with the option.

On second thought, his probably doesn't affect your code because the
implementation is working at the COFF section level, not the XCOFF
storage mapping class level.

>
> Moreover, if it is not implemented on AIX, don't you think that such an 
> option which is documented as:
> " Place each function or data item into its own section in the output file if 
> the
> target supports arbitrary sections. The name of the function or the name of
> the data item determines the section’s name in the output file.
> Use these options on systems where the linker can perform optimizations to
> improve locality of reference in the instruction space. Most systems using the
> ELF object format and SPARC processors running Solaris 2 have linkers with
> such optimizations. AIX may have these optimizations in the future.
> Only use these options when there are significant benefits from doing so. When
> you specify these options, the assembler and linker create larger object and
> executable files and are also slower."
> is not compatible with the already existing high complexity of GCC Go 
> implementation ?and should be forbidden with Go on AIX ?
>
>
> We have tried another approach:
> 127a128
>> #define STYP_TEXT 0x20
> 408a410
>>   unsigned int flags;
> 482a485,486
>> flags = fetch_32 (scnhdr + offsetof (struct external_scnhdr,
>>  u.xcoff64.s_flags));
> 489a494,495
>> flags = fetch_32 (scnhdr + offsetof (struct external_scnhdr,
>>  u.xcoff32.s_flags));
> 492c498
> <   if (strcmp (name, ".text") == 0)
> ---
>>   if ((flags & 0x) == STYP_TEXT)
>
> However, that makes never-seen-before errors to appear when running libgo 
> tests in always-succeeding libgo tests, like: bufio & bytes.
>
>
> Since we have many other GCC Go stuff on AIX to handle, wouldn't it be 
> possible to start with this implementation and to improve it later if it 
> needs to be hardened ?
> Document it as a limitation.

As I wrote in my original reply, it's an incremental start.

".text" may be sufficient.  We'll see.

Thanks, David

RE:[PATCH,AIX] Enable libiberty to read AIX XCOFF

[REIX, Tony]

Hi David,

I'll fix the code incorrectly indented.

About your comment about our code looking for TEXT section by looking at string 
".text"  , please note that our patch fixes a file called 
"simple-object-xcoff.c" : SIMPLE.
Do not expect us to handle more than required.

However, are you sure that  -ffunction-sections  is implemented on AIX ? 

Moreover, if it is not implemented on AIX, don't you think that such an option 
which is documented as:
" Place each function or data item into its own section in the output file if 
the
target supports arbitrary sections. The name of the function or the name of
the data item determines the section’s name in the output file.
Use these options on systems where the linker can perform optimizations to
improve locality of reference in the instruction space. Most systems using the
ELF object format and SPARC processors running Solaris 2 have linkers with
such optimizations. AIX may have these optimizations in the future.
Only use these options when there are significant benefits from doing so. When
you specify these options, the assembler and linker create larger object and
executable files and are also slower."
is not compatible with the already existing high complexity of GCC Go 
implementation ?and should be forbidden with Go on AIX ?


We have tried another approach:
127a128
> #define STYP_TEXT 0x20
408a410
>   unsigned int flags;
482a485,486
> flags = fetch_32 (scnhdr + offsetof (struct external_scnhdr,
>  u.xcoff64.s_flags));
489a494,495
> flags = fetch_32 (scnhdr + offsetof (struct external_scnhdr,
>  u.xcoff32.s_flags));
492c498
<   if (strcmp (name, ".text") == 0)
---
>   if ((flags & 0x) == STYP_TEXT)

However, that makes never-seen-before errors to appear when running libgo tests 
in always-succeeding libgo tests, like: bufio & bytes.


Since we have many other GCC Go stuff on AIX to handle, wouldn't it be possible 
to start with this implementation and to improve it later if it needs to be 
hardened ?
Document it as a limitation.


Regards,

Cordialement,

Tony Reix

Bull - ATOS
IBM Coop Architect & Technical Leader
Office : +33 (0) 4 76 29 72 67
1 rue de Provence - 38432 Échirolles - France
www.atos.net


De : David Edelsohn [dje@gmail.com]
Envoyé : mercredi 7 juin 2017 01:25
À : REIX, Tony; Ian Taylor
Cc : SARTER, MATTHIEU (ext); GCC Patches
Objet : Re: [PATCH,AIX] Enable libiberty to read AIX XCOFF

Tony,

This patch generally looks good to me -- it clearly is an incremental
improvement.  One of the libiberty maintainers, such as Ian, needs to
approve the patch.

https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01181.html

+  if (strcmp (name, ".text") == 0)
+textptr = scnptr;

The above code does not seem very robust.  What if the application is
compiled with -ffunction-sections so the text section is not named
".text"?

+  if (strtab == NULL)
+{
+ XDELETEVEC (symtab);
+  XDELETEVEC (scnbuf);
+  return errmsg;

The first XDELETEVEC (symtab) is indented incorrectly and should be fixed.

Thanks, David

RE:[PATCH,AIX] Enable libiberty to read AIX XCOFF

[REIX, Tony]

Hi DJ

A) XNEWVEC

1) ./include/libiberty.h:

It appears that XNEWVEC() calls xmalloc which prints a message and calls xexit 
if malloc fails.

#define XNEWVEC(T, N) ((T *) xmalloc (sizeof (T) * (N)))

/* Allocate memory without fail.  If malloc fails, this will print a
   message to stderr (using the name set by xmalloc_set_program_name,
   if any) and then call xexit.  */
extern void *xmalloc (size_t) ATTRIBUTE_MALLOC ATTRIBUTE_RETURNS_NONNULL;


2)  ./libiberty/simple-object-xcoff.c :

It appears that  XNEWVEC() was already called 2 times before we added a third 
use of it, and still with NO check of return.

simple_object_xcoff_read_strtab (...)
{
...
  strtab = XNEWVEC (char, strsize);
  if (!simple_object_internal_read (sobj->descriptor, strtab_offset,
(unsigned char *) strtab, strsize, errmsg,
err))
...

simple_object_xcoff_find_sections (...)
{
 ...
  scnbuf = XNEWVEC (unsigned char, scnhdr_size * ocr->nscns);
  if (!simple_object_internal_read (sobj->descriptor,
sobj->offset + ocr->scnhdr_offset,
scnbuf, scnhdr_size * ocr->nscns, ,
err))


Thus, I think that we should continue to do what we did and do NOT check the 
return of XNEWVEC() .



B) XDELETEVEC

1) ./include/libiberty.h:

#define XDELETEVEC(P) free ((void*) (P))


2) free() documentation : The free subroutine deallocates a  ... If the Pointer 
parameter is NULL, no action occurs.


So, yes, we check   if (strtab == NULL)   though there is no way that 
XDELETEVEC(NULL) breaks something.
However, it is a classic programming style.

And the same programming style was used before we added our patch in 
simple_object_xcoff_find_sections () :
  /* The real section name is found in the string
 table.  */
  if (strtab == NULL)
{
  strtab = simple_object_xcoff_read_strtab (sobj,
   _size,
   , err);
  if (strtab == NULL)
{
  XDELETEVEC (scnbuf);
  return errmsg;
}
}

So our new code seems coherent with previous existing code.


Regards,

Cordialement,

Tony Reix

Bull - ATOS
IBM Coop Architect & Technical Leader
Office : +33 (0) 4 76 29 72 67
1 rue de Provence - 38432 Échirolles - France
www.atos.net


De : DJ Delorie [d...@redhat.com]
Envoyé : mercredi 7 juin 2017 01:52
À : David Edelsohn
Cc : REIX, Tony; i...@golang.org; SARTER, MATTHIEU (ext); 
gcc-patches@gcc.gnu.org
Objet : Re: [PATCH,AIX] Enable libiberty to read AIX XCOFF

David Edelsohn <dje@gmail.com> writes:
> This patch generally looks good to me -- it clearly is an incremental
> improvement.  One of the libiberty maintainers, such as Ian, needs to
> approve the patch.

As AIX maintainer, I think you have the authority to approve patches
like this, which only affect your OS.  I see no reason to reject the
patch myself, other than:

+  symtab = XNEWVEC (struct external_syment, ocr->nsyms * SYMESZ);
+  if (!simple_object_internal_read (sobj->descriptor,

There's no check to see if XNEWVEC succeeded.


Also, the use of XDELETEVEC is inconsistently protected with a "if (foo
!= NULL)" throughout, but passing NULL to XDELETEVEC (essentially,
free()) is allowed anyway, so this is only a stylistic issue, which I'm
not particularly worried about.

Re: [PATCH,AIX] Enable libiberty to read AIX XCOFF

[DJ Delorie]

David Edelsohn  writes:
> This patch generally looks good to me -- it clearly is an incremental
> improvement.  One of the libiberty maintainers, such as Ian, needs to
> approve the patch.

As AIX maintainer, I think you have the authority to approve patches
like this, which only affect your OS.  I see no reason to reject the
patch myself, other than:

+  symtab = XNEWVEC (struct external_syment, ocr->nsyms * SYMESZ);
+  if (!simple_object_internal_read (sobj->descriptor,

There's no check to see if XNEWVEC succeeded.


Also, the use of XDELETEVEC is inconsistently protected with a "if (foo
!= NULL)" throughout, but passing NULL to XDELETEVEC (essentially,
free()) is allowed anyway, so this is only a stylistic issue, which I'm
not particularly worried about.

Re: [PATCH,AIX] Enable libiberty to read AIX XCOFF

[David Edelsohn]

Tony,

This patch generally looks good to me -- it clearly is an incremental
improvement.  One of the libiberty maintainers, such as Ian, needs to
approve the patch.

https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01181.html

+  if (strcmp (name, ".text") == 0)
+textptr = scnptr;

The above code does not seem very robust.  What if the application is
compiled with -ffunction-sections so the text section is not named
".text"?

+  if (strtab == NULL)
+{
+ XDELETEVEC (symtab);
+  XDELETEVEC (scnbuf);
+  return errmsg;

The first XDELETEVEC (symtab) is indented incorrectly and should be fixed.

Thanks, David

[PATCH,AIX] Enable libiberty to read AIX XCOFF

[REIX, Tony]

Description:
 * This patch enables libiberty to read AIX XCOFF.

Tests:
 * Fedora25/x86_64 + GCC v7.1.0 : Configure/Build: SUCCESS
   - build made by means of a .spec file based on Fedora gcc-7.0.1-0.12 .spec 
file
 ../configure --enable-bootstrap 
--enable-languages=c,c++,objc,obj-c++,fortran,go,lto --prefix=/usr 
--mandir=/usr/share/man --infodir=/usr/share/info 
--with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared 
--enable-threads=posix --enable-checking=release
 --enable-multilib --with-system-zlib --enable-__cxa_atexit 
--disable-libunwind-exceptions --enable-gnu-unique-object 
--enable-linker-build-id --with-gcc-major-version-only 
--with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl 
--enable-libmpx
 --enable-offload-targets=nvptx-none --without-cuda-driver 
--enable-gnu-indirect-function --with-tune=generic --with-arch_32=i686 
--build=x86_64-redhat-linux

ChangeLog:
  * libiberty/simple-object-xcoff.c: Enable libiberty to read AIX XCOFF


Regards,

Tony Reix
Bull - ATOS
IBM Coop Architect & Technical Leader
Office : +33 (0) 4 76 29 72 67
1 rue de Provence - 38432 Échirolles - France
www.atos.net
--- ./libiberty/simple-object-xcoff.c.ORIGIN2017-03-21 17:08:59 -0500
+++ ./libiberty/simple-object-xcoff.c   2017-03-21 16:45:43 -0500
@@ -258,6 +258,8 @@
 #define C_STAT (3)
 #define C_FILE (103)
 
+#define DBXMASK0x80
+
 /* Private data for an simple_object_read.  */
 
 struct simple_object_xcoff_read
@@ -403,7 +405,9 @@
   unsigned int nscns;
   char *strtab;
   size_t strtab_size;
+  struct external_syment *symtab = NULL;
   unsigned int i;
+  off_t textptr = 0;
 
   scnhdr_size = u64 ? SCNHSZ64 : SCNHSZ32;
   scnbuf = XNEWVEC (unsigned char, scnhdr_size * ocr->nscns);
@@ -485,10 +489,116 @@
  u.xcoff32.s_size));
}
 
+  if (strcmp (name, ".text") == 0)
+textptr = scnptr;
   if (!(*pfn) (data, name, scnptr, size))
break;
 }
 
+  /* Special handling for .go_export CSECT. */
+  if (textptr != 0 && ocr->nsyms > 0)
+{
+  unsigned char *sym, *aux;
+  const char *n_name;
+  unsigned long n_value, n_offset, n_zeroes, x_scnlen;
+
+  /* Read symbol table. */
+  symtab = XNEWVEC (struct external_syment, ocr->nsyms * SYMESZ);
+  if (!simple_object_internal_read (sobj->descriptor,
+sobj->offset + ocr->symptr,
+(unsigned char *)symtab,
+ocr->nsyms * SYMESZ,
+, err))
+{
+  XDELETEVEC (symtab);
+ XDELETEVEC (scnbuf);
+  return NULL;
+}
+  /* Search in symbol table if we have a ".go_export" symbol. */
+  for (i = 0; i < ocr->nsyms; ++i)
+{
+  sym = (unsigned char *)[i];
+
+  if (symtab[i].n_sclass[0] & DBXMASK)
+{
+  /* Skip debug symbols whose names are in stabs. */
+  i += symtab[i].n_numaux[0];
+  continue;
+}
+  if (u64)
+{
+  n_value = fetch_64 (sym + offsetof (struct external_syment,
+  u.xcoff64.n_value));
+  n_offset = fetch_32 (sym + offsetof (struct external_syment,
+   u.xcoff64.n_offset));
+}
+  else
+{
+  /* ".go_export" is longer than N_SYMNMLEN */
+  n_zeroes = fetch_32 (sym + offsetof (struct external_syment,
+   u.xcoff32.n.n.n_zeroes));
+  if (n_zeroes != 0)
+{
+  /* Skip auxiliary entries. */
+  i += symtab[i].n_numaux[0];
+  continue;
+}
+  n_value = fetch_32 (sym + offsetof (struct external_syment,
+  u.xcoff32.n_value));
+  n_offset = fetch_32 (sym + offsetof (struct external_syment,
+   u.xcoff32.n.n.n_offset));
+}
+ /* The real section name is found in the string
+table.  */
+ if (strtab == NULL)
+   {
+ strtab = simple_object_xcoff_read_strtab (sobj,
+   _size,
+   , err);
+ if (strtab == NULL)
+   {
+  XDELETEVEC (symtab);
+ XDELETEVEC (scnbuf);
+ return errmsg;
+   }
+   }
+
+ if (n_offset >= strtab_size)
+{
+ XDELETEVEC (strtab);
+ XDELETEVEC (symtab);
+ XDELETEVEC (scnbuf);
+ *err = 0;
+ return "section string index out of range";
+