[v4,00/35] CTF linking support

Message ID 20190924135131.441906-1-nick.alcock@oracle.com
Headers show
Series
  • CTF linking support
Related show

Message

Nick Alcock Sept. 24, 2019, 1:50 p.m.
This is v4 of the CTF linking patch series, with many bugfixes since the
last submission:
 - check for strdup failure and OOM in many more places
 - handle OOM in dumping better (omitting pieces of the dump, rather
   than crashing)
 - make the CTF compression threshold a #define
 - avoid the need to modify the linker script
 - make it possible to free CTF sections like other in-memory sections
   can be freed
 - make into a shared library (well, two: one with BFD built in, one
   not) in the same way as libopcodes, controlled by the same configure
   flag, using versioned symbols on platforms that support
   ld --version-script
 - drop elf-bfd.h hackery
 - adjustment to recent upstream changes (ldelf, etc)
 - drop ctf_malloc, ctf_free, and ctf_strdup wrappers, and fix a minor
   refcount leak discovered in the process
 - fix an uninitialized variable read in ctf_add_type causing massive
   unnecessary duplication of types

No regressions in make check on any platform I have access to (the usual
mix of x86_64-pc-linux-gnu, FreeBSD 12, Solaris, SPARCLinux, and mingw);
no regressions in make check with -gt forced on except for ones that are
necessarily there because of the introduction of a new section in the
output without regenerating the expected output.  Also tested every
combination of --enable-shared, --enable-static, --disable-shared,
--disable-static, and --with-system-zlib that I could think of until
nothing broke.

I Cc:ed reviewers of patches on the changed versions of each patch they
reviewed. If this is considered impolite, let me know and I'll stop
doing it.  (Patches where I changed things for reasons not triggered
directly by reviews are not Cc:ed to anyone.)

I'll probably try some full-distro builds next, both with -gt off and
with it on. :)

I believe every review comment has been incorporated.  Commits that
changed in this series are marked CHANGED in the mail subject.  Commits
that are new in this series are marked NEW.

Current CTF size example:

oranix@loom 551 % size -A /tmp/gcc/bin/ld
ld/.libs/ld-new  :
section                 size      addr
.interp                   28   4194984
.note.gnu.build-id        36   4195012
.note.ABI-tag             32   4195048
.gnu.hash                212   4195080
.dynsym                 5640   4195296
.dynstr                 3625   4200936
.gnu.version             470   4204562
.gnu.version_r           112   4205032
.rela.dyn                360   4205144
.rela.plt               5088   4205504
.init                     26   4210688
.plt                    3408   4210720
.plt.got                   8   4214128
.text                 177954   4214144
.fini                      9   4392100
.rodata              1211577   4395008
.eh_frame_hdr           3972   5606588
.eh_frame              23200   5610560
.init_array                8   5638832
.fini_array                8   5638840
.data.rel.ro             768   5638848
.dynamic                 528   5639616
.got                      32   5640144
.got.plt                1720   5640192
.data                   2368   5641920
.bss                    5304   5644288
.comment                  91         0
.ctf                  162220         0
Total                1608804


(CTF down from 388104 bytes in the last review push: the addr now also
 correctly shows as 0 for this non-loaded section, probably due to the
 excision of .ctf from the default linker script)

oranix@loom 552 % PATH=/tmp/gcc/bin:$PATH objdump --ctf=.ctf /tmp/gcc/bin/ld

/tmp/gcc/bin/ld:     file format elf64-x86-64

Contents of CTF section .ctf:

  Header:
    Magic number: dff2
    Version: 4 (CTF_VERSION_3)
    Flags: 0x1 (CTF_F_COMPRESS)
    Variable section:   0x0 -- 0x7b7 (0x7b8 bytes)
    Type section:       0x7b8 -- 0x9bec7 (0x9b710 bytes)
    String section:     0x9bec8 -- 0xb343d (0x17576 bytes)

  Labels:

  Data objects:

  Function objects:

  Variables:
    _IO_2_1_stderr_ ->  6e: struct _IO_FILE_plus (size 0x6)
    _IO_2_1_stdin_ ->  6e: struct _IO_FILE_plus (size 0x6)
    _IO_2_1_stdout_ ->  6e: struct _IO_FILE_plus (size 0x6)
    __daylight ->  10: int (size 0x4)
    __environ ->  179: char ** (size 0x8) -> 178: char * (size 0x8) -> 44: char (size 0x1)
    __timezone ->  13: long int (size 0x8)
    __tzname ->  79ff: char *[2] (size 0x10)
    _bfd_elf_large_com_section ->  1d1: struct bfd_section (size 0x118)
    _bfd_std_section ->  217: struct bfd_section [4] (size 0x460)
    _hex_value ->  9ca: const unsigned char [256] (size 0x100)
    _sch_istable ->  9c7: const short unsigned int [256] (size 0x200)
    _sch_tolower ->  9ca: const unsigned char [256] (size 0x100)
[...]

     1: long unsigned int (size 0x8)
        [0x0] (ID 0x1) (kind 1) long unsigned int  (aligned at 0x8, format 0x0, offset:bits 0x0:0x40)
     2: size_t (size 0x8) -> 1: long unsigned int (size 0x8)
        [0x0] (ID 0x2) (kind 10) size_t  (aligned at 0x8)
     3: unsigned char (size 0x1)
        [0x0] (ID 0x3) (kind 1) unsigned char  (aligned at 0x1, format 0x2, offset:bits 0x0:0x8)
     4: __u_char (size 0x1) -> 3: unsigned char (size 0x1)
        [0x0] (ID 0x4) (kind 10) __u_char  (aligned at 0x1)
     5: short unsigned int (size 0x2)
        [0x0] (ID 0x5) (kind 1) short unsigned int  (aligned at 0x2, format 0x0, offset:bits 0x0:0x10)
     6: __u_short (size 0x2) -> 5: short unsigned int (size 0x2)
        [0x0] (ID 0x6) (kind 10) __u_short  (aligned at 0x2)
     7: unsigned int (size 0x4)
        [0x0] (ID 0x7) (kind 1) unsigned int  (aligned at 0x4, format 0x0, offset:bits 0x0:0x20)
     8: __u_int (size 0x4) -> 7: unsigned int (size 0x4)
        [0x0] (ID 0x8) (kind 10) __u_int  (aligned at 0x4)
     9: __u_long (size 0x8) -> 1: long unsigned int (size 0x8)
        [0x0] (ID 0x9) (kind 10) __u_long  (aligned at 0x8)
     a: signed char (size 0x1)
        [0x0] (ID 0xa) (kind 1) signed char  (aligned at 0x1, format 0x3, offset:bits 0x0:0x8)
     b: __int8_t (size 0x1) -> a: signed char (size 0x1)
        [0x0] (ID 0xb) (kind 10) __int8_t  (aligned at 0x1)
[...]
     7a60: ucontext_t (size 0x3a8) -> 7a5d: struct ucontext (size 0x3a8)
        [0x0] (ID 0x7a60) (kind 10) ucontext_t  (aligned at 0x8)
            [0x0] (ID 0x1) (kind 1) long unsigned int uc_flags (aligned at 0x8, format 0x0, offset:bits 0x0:0x40)
            [0x40] (ID 0x7a5e) (kind 3) struct ucontext * uc_link (aligned at 0x8)
            [0x80] (ID 0x7a40) (kind 6) struct sigaltstack uc_stack (aligned at 0x8)
                [0x80] (ID 0x79b9) (kind 3) void * ss_sp (aligned at 0x8)
                [0xc0] (ID 0x10) (kind 1) int ss_flags (aligned at 0x4, format 0x1, offset:bits 0x0:0x20)
                [0x100] (ID 0x1) (kind 1) long unsigned int ss_size (aligned at 0x8, format 0x0, offset:bits 0x0:0x40)
            [0x140] (ID 0x7a5b) (kind 6) struct  uc_mcontext (aligned at 0x8)
                [0x140] (ID 0x7a44) (kind 4) long long int [23] gregs (aligned at 0x8)
                [0x700] (ID 0x7a56) (kind 3) struct _libc_fpstate * fpregs (aligned at 0x8)
                [0x740] (ID 0x7a5a) (kind 4) long long unsigned int [8] __reserved1 (aligned at 0x8)
            [0x940] (ID 0x79be) (kind 6) struct  uc_sigmask (aligned at 0x8)
                [0x940] (ID 0x79bc) (kind 4) long unsigned int [16] __val (aligned at 0x8)
            [0xd40] (ID 0x7a46) (kind 6) struct _libc_fpstate __fpregs_mem (aligned at 0x2)
                [0xd40] (ID 0x5) (kind 1) short unsigned int cwd (aligned at 0x2, format 0x0, offset:bits 0x0:0x10)
                [0xd50] (ID 0x5) (kind 1) short unsigned int swd (aligned at 0x2, format 0x0, offset:bits 0x0:0x10)
                [0xd60] (ID 0x5) (kind 1) short unsigned int ftw (aligned at 0x2, format 0x0, offset:bits 0x0:0x10)
                [0xd70] (ID 0x5) (kind 1) short unsigned int fop (aligned at 0x2, format 0x0, offset:bits 0x0:0x10)
                [0xd80] (ID 0x1) (kind 1) long unsigned int rip (aligned at 0x8, format 0x0, offset:bits 0x0:0x40)
                [0xdc0] (ID 0x1) (kind 1) long unsigned int rdp (aligned at 0x8, format 0x0, offset:bits 0x0:0x40)
                [0xe00] (ID 0x7) (kind 1) unsigned int mxcsr (aligned at 0x4, format 0x0, offset:bits 0x0:0x20)
                [0xe20] (ID 0x7) (kind 1) unsigned int mxcr_mask (aligned at 0x4, format 0x0, offset:bits 0x0:0x20)
                [0xe40] (ID 0x7a4a) (kind 4) struct _libc_fpxreg [8] _st (aligned at 0x2)
                [0x1240] (ID 0x7a4d) (kind 4) struct _libc_xmmreg [16] _xmm (aligned at 0x4)
                [0x1a40] (ID 0x7a4e) (kind 4) unsigned int [24] padding (aligned at 0x4)
     7a61: char * (size 0x8) -> 44: char (size 0x1)
        [0x0] (ID 0x7a61) (kind 3) char *  (aligned at 0x8)
     7a62: char ** (size 0x8) -> 7a61: char * (size 0x8) -> 44: char (size 0x1)
        [0x0] (ID 0x7a62) (kind 3) char **  (aligned at 0x8)
     7a63: long int () (size 0x0)
        [0x0] (ID 0x7a63) (kind 5) long int ()  (aligned at 0x8)

Strings:
    0: 
    1: A
    3: AARCH64_ELF_DATA
    14: ALPHA_ELF_DATA
    23: ARC_ELF_DATA
    30: ARM_ELF_DATA
    3d: AVR_ELF_DATA
    4a: B
    4c: BFD_ARELOC_BFIN_ADD
    60: BFD_ARELOC_BFIN_ADDR
    75: BFD_ARELOC_BFIN_AND
    89: BFD_ARELOC_BFIN_COMP
    9e: BFD_ARELOC_BFIN_CONST
    b4: BFD_ARELOC_BFIN_DIV
    c8: BFD_ARELOC_BFIN_HWPAGE
    df: BFD_ARELOC_BFIN_LAND
[...]

Hans-Peter Nilsson (1):
  libctf: make it compile for old glibc

Nick Alcock (34):
  libctf: allow the header to change between versions
  libctf, binutils: dump the CTF header
  libctf, bfd: fix ctf_bfdopen_ctfsect opening symbol and string
    sections
  libctf: add the object index and function index sections
  binutils: readelf: when dumping CTF, load strtab and symtab
    automatically
  binutils: objdump does not take --ctf-symbols or --ctf-strings options
  libctf: Add iteration over non-root types
  libctf: support getting strings from the ELF strtab
  libctf: write CTF files to memory, and CTF archives to fds
  libctf: fix double-free on ctf_compress_write error path
  libctf: dump: support non-root type dumping
  libctf: dump: check the right error values when dumping functions
  libctf: add the ctf_link machinery
  libctf: map from old to corresponding newly-added types in
    ctf_add_type
  libctf: add linking of the variable section
  libctf: add CU-mapping machinery
  libctf: teach ctf_add_type how forwards work
  libctf: don't leak hash keys or values on value replacement
  libctf: eschew C99 for loop initial declarations
  libctf: get rid of a disruptive public include of <sys/param.h>
  libctf: bfd-open: mark the bfd as cacheable
  libctf: actually close bfds we have opened
  bfd: new functions for getting strings out of a strtab
  bfd, ld: add CTF section linking
  libctf: installable libctf as a shared library
  objdump: get CTF parent importing right
  libctf: handle nonrepresentable types at link time
  libctf: avoid the need to ever use ctf_update
  libctf: properly handle ctf_add_type of forwards and self-reffing
    structs
  libctf: allow ctf_type_lname of a null pointer.
  libctf: get the encoding of non-ints/fps in the dynamic space right
  libctf: remove ctf_malloc, ctf_free and ctf_strdup
  libctf: make ctf_dump not crash on OOM
  libctf: fix refcount leak in ctf_import

-- 
2.23.0.239.g28aa4420fd

Comments

Alan Modra Sept. 25, 2019, 9:20 a.m. | #1
So I applied this series and the first thing I noticed was a whole lot
of "git am" complaints.  In my .git/config I have
[core]
...
        whitespace = indent-with-non-tab,space-before-tab,trailing-space
to help keep my own patches consistent with how most binutils files
are formatted.  Please don't introduce more inconsistencies.

The second problem I ran into was that --enable-maintainer-mode
regenerated libctf autotools files without AM_INSTALL_LIBBFD being
expanded, resulting in a build failure.  The following should fix that
problem.

diff --git a/libctf/Makefile.am b/libctf/Makefile.am
index d21f730cc2..206e0df877 100644
--- a/libctf/Makefile.am
+++ b/libctf/Makefile.am
@@ -17,7 +17,7 @@
 # <http://www.gnu.org/licenses/>.
 #
 
-ACLOCAL_AMFLAGS = -I .. -I ../config
+ACLOCAL_AMFLAGS = -I .. -I ../config -I ../bfd
 
 AUTOMAKE_OPTIONS = foreign no-texinfo.tex
 
diff --git a/libctf/configure.ac b/libctf/configure.ac
index 575df7183c..6864980103 100644
--- a/libctf/configure.ac
+++ b/libctf/configure.ac
@@ -22,6 +22,7 @@ AC_PREREQ(2.64)
 AC_INIT([libctf library], 1.2.0-pre)
 AC_CONFIG_SRCDIR(ctf-impl.h)
 AC_CONFIG_MACRO_DIR(../config)
+AC_CONFIG_MACRO_DIR(../bfd)
 AC_USE_SYSTEM_EXTENSIONS
 AM_INIT_AUTOMAKE
 
With that everything built and tests on lots of targets were without
regression.  Then, just for fun I tried configuring with
--disable-libctf.
make[4]: Entering directory '/home/alan/build/gas/tmp/binutils'
make[4]: *** No rule to make target '../libctf/libctf.la', needed by 'objdump'.  Stop.
That's not a show-stopper, just something that might be nice to have
in the future.

Now some comments on individual patches.

> +/* Determine if a section contains CTF data, using its name.  */

> +#define SECTION_IS_CTF(name) \

> +	(strcmp ((name), ".ctf") == 0 || strncmp ((name), ".ctf.", 5) == 0)

This would be better if the macro argument was the section rather than
the section name.  Even better,

static inline bfd_boolean
bfd_section_is_ctf (const asection *sec)
{
  const char *name = bfd_section_name (sec);
  return strncmp (name, ".ctf", 4) == 0 && (name[4] == 0 || name[4] == '.');
}

> -		   && (hdr->bfd_section->flags & SEC_ELF_COMPRESS))

> -		   /* Compress DWARF debug sections.  */

> +		   && (hdr->bfd_section->flags & SEC_ELF_COMPRESS

> +		       || (SECTION_IS_CTF (hdr->bfd_section->name)

> +			   && abfd->is_linker_output)))

> +		   /* We don't know the offset of these sections yet: their size

> +                      has not been decided.  */

Place comment before the code, not after.  Yes, I know, there was
already a misplaced comment there.

> -		  && (hdr->bfd_section->flags & SEC_ELF_COMPRESS))

> -		  /* Compress DWARF debug sections.  */

> +		  && (hdr->bfd_section->flags & SEC_ELF_COMPRESS

> +		      || (SECTION_IS_CTF (hdr->bfd_section->name)

> +			  && abfd->is_linker_output)))

> +              /* Do not assign offsets for these sections yet: we don't know

> +                 their sizes.  */

Ditto.

> --- a/ld/ldelfgen.h

> +++ b/ld/ldelfgen.h

> @@ -18,4 +18,15 @@

>     Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,

>     MA 02110-1301, USA.  */

>  

> +#include "sysdep.h"

> +#include "bfd.h"

> +#include "ctf-api.h"

In ld/ we've gone for a more-or-less flat includes, ie. headers
generally don't include other headers.  Please add the necessary
includes (and only the necessary includes) before occurrences of
ldelfgen.h.  Note that files like vms.em are sourceed from generic.em
and thus will already include some headers.

> --- a/ld/ldlang.h

> +++ b/ld/ldlang.h

> @@ -21,6 +21,8 @@

>  #ifndef LDLANG_H

>  #define LDLANG_H

>  

> +#include "ctf-api.h"

> +

Ditto here.

>  #define DEFAULT_MEMORY_REGION   "*default*"

>  

>  typedef enum

> @@ -296,6 +298,8 @@ typedef struct lang_input_statement_struct

>  

>    bfd *the_bfd;

>  

> +  ctf_archive_t *the_ctf;

> +

Maybe this could be a void*, to reduce the number of places you would
otherwise need to add crf-api.h?

With these comments addressed (minus --disable-libctf if you like) the
patch series looks good to go.  In reviewing, I admit I have only
given a quick glance over the libctf code, but that's your baby as far
as I'm concerned.

-- 
Alan Modra
Australia Development Lab, IBM
Nick Alcock Sept. 25, 2019, 1:59 p.m. | #2
On 25 Sep 2019, Alan Modra uttered the following:

> So I applied this series and the first thing I noticed was a whole lot

> of "git am" complaints.  In my .git/config I have

> [core]

> ...

>         whitespace = indent-with-non-tab,space-before-tab,trailing-space

> to help keep my own patches consistent with how most binutils files

> are formatted.  Please don't introduce more inconsistencies.


These are all from generated code, introduced by Automake: the source
has no trailing whitespace.

I am at a loss how to fix this, except by post-facto editing of the
Makefile.am and configure script, which seems pointless since the next
regeneration will just overwrite it again.

> The second problem I ran into was that --enable-maintainer-mode

> regenerated libctf autotools files without AM_INSTALL_LIBBFD being

> expanded, resulting in a build failure.  The following should fix that

> problem.


Oh, good. I had no idea how to make aclocal work post-libtoolization and
was working by manually adding ac_include lines to aclocal.m4: whenever
I try running aclocal it removes all the ac_includes and substitutes
them in instead, leading to massive unnecessary diffs. So
aclocal-related problems are... not a surprise. (I was operating under
the assuption that nobody with libtoolized subprojects was ever running
aclocal on them, but was just working by manually copying things into
../config and adding ac_include's for them as needed.)

> diff --git a/libctf/Makefile.am b/libctf/Makefile.am

> index d21f730cc2..206e0df877 100644

> --- a/libctf/Makefile.am

> +++ b/libctf/Makefile.am

> @@ -17,7 +17,7 @@

>  # <http://www.gnu.org/licenses/>.

>  #

>  

> -ACLOCAL_AMFLAGS = -I .. -I ../config

> +ACLOCAL_AMFLAGS = -I .. -I ../config -I ../bfd

>  

>  AUTOMAKE_OPTIONS = foreign no-texinfo.tex

>  

> diff --git a/libctf/configure.ac b/libctf/configure.ac

> index 575df7183c..6864980103 100644

> --- a/libctf/configure.ac

> +++ b/libctf/configure.ac

> @@ -22,6 +22,7 @@ AC_PREREQ(2.64)

>  AC_INIT([libctf library], 1.2.0-pre)

>  AC_CONFIG_SRCDIR(ctf-impl.h)

>  AC_CONFIG_MACRO_DIR(../config)

> +AC_CONFIG_MACRO_DIR(../bfd)

>  AC_USE_SYSTEM_EXTENSIONS

>  AM_INIT_AUTOMAKE


Thanks, will apply.

> With that everything built and tests on lots of targets were without

> regression.  Then, just for fun I tried configuring with

> --disable-libctf.


... you can do that?! In decades of using Cygnus-tree projects I never
realised you could disable subdirs at will like this (it doesn't appear
in configure --help either). So I never tested it. :)

> make[4]: Entering directory '/home/alan/build/gas/tmp/binutils'

> make[4]: *** No rule to make target '../libctf/libctf.la', needed by 'objdump'.  Stop.

> That's not a show-stopper, just something that might be nice to have

> in the future.


Oh yeah, definitely, and I think I see how to fix it too.

>> +/* Determine if a section contains CTF data, using its name.  */

>> +#define SECTION_IS_CTF(name) \

>> +	(strcmp ((name), ".ctf") == 0 || strncmp ((name), ".ctf.", 5) == 0)

> This would be better if the macro argument was the section rather than

> the section name.  Even better,

>

> static inline bfd_boolean

> bfd_section_is_ctf (const asection *sec)

> {

>   const char *name = bfd_section_name (sec);

>   return strncmp (name, ".ctf", 4) == 0 && (name[4] == 0 || name[4] == '.');

> }


Much nicer! Will fix.

>> -		   && (hdr->bfd_section->flags & SEC_ELF_COMPRESS))

>> -		   /* Compress DWARF debug sections.  */

>> +		   && (hdr->bfd_section->flags & SEC_ELF_COMPRESS

>> +		       || (SECTION_IS_CTF (hdr->bfd_section->name)

>> +			   && abfd->is_linker_output)))

>> +		   /* We don't know the offset of these sections yet: their size

>> +                      has not been decided.  */

> Place comment before the code, not after.  Yes, I know, there was

> already a misplaced comment there.


Serves me right for not noticing the surrounding context :) will fix.

>> --- a/ld/ldelfgen.h

>> +++ b/ld/ldelfgen.h

>> @@ -18,4 +18,15 @@

>>     Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,

>>     MA 02110-1301, USA.  */

>>  

>> +#include "sysdep.h"

>> +#include "bfd.h"

>> +#include "ctf-api.h"

> In ld/ we've gone for a more-or-less flat includes, ie. headers

> generally don't include other headers.  Please add the necessary

> includes (and only the necessary includes) before occurrences of

> ldelfgen.h.  Note that files like vms.em are sourceed from generic.em

> and thus will already include some headers.


I tried that the first time (with ctf-api.h, below) and it touched so
many files I suspected you'd reject it out of hand and reverted it
before submission: it's ridiculously tricky to get right in the presence
of the .em-driven machinery. I honestly wonder why this include style is
considered sensible for *any* project, let alone this one, but... ok,
will change. (There are only four cases anyway.)

(IMHO, banning headers from including other headers makes about as much
sense as banning functions from calling other functions. It's the enemy
of functional decomposition and information hiding. But so be it: it's
the way ld works now, and if we change it it shouldn't be done
piecemeal in the middle of other patches.)

>> --- a/ld/ldlang.h

>> +++ b/ld/ldlang.h

>> @@ -21,6 +21,8 @@

>>  #ifndef LDLANG_H

>>  #define LDLANG_H

>>  

>> +#include "ctf-api.h"

>> +

> Ditto here.

>

>>  #define DEFAULT_MEMORY_REGION   "*default*"

>>  

>>  typedef enum

>> @@ -296,6 +298,8 @@ typedef struct lang_input_statement_struct

>>  

>>    bfd *the_bfd;

>>  

>> +  ctf_archive_t *the_ctf;

>> +

> Maybe this could be a void*, to reduce the number of places you would

> otherwise need to add crf-api.h?


In theory, but this really does feel like damaging the system's clarity
for the sake of a stylistic convention whose purpose is... less than
clear to me. It's *not* a void *, and making it one may hide bugs.

However, I think I can get away with calling it a 'struct ctf_file' and
using a forward.

> With these comments addressed (minus --disable-libctf if you like) the

> patch series looks good to go.  In reviewing, I admit I have only

> given a quick glance over the libctf code, but that's your baby as far

> as I'm concerned.


Even a quick glance is better than nothing!

I'll repost again once these are resolved, since there will obviously be
changes, but it's nice to know I'm getting there.

(And thank you!)
Alan Modra Sept. 25, 2019, 2:38 p.m. | #3
On Wed, Sep 25, 2019 at 02:59:43PM +0100, Nick Alcock wrote:
> On 25 Sep 2019, Alan Modra uttered the following:

> 

> > So I applied this series and the first thing I noticed was a whole lot

> > of "git am" complaints.  In my .git/config I have

> > [core]

> > ...

> >         whitespace = indent-with-non-tab,space-before-tab,trailing-space

> > to help keep my own patches consistent with how most binutils files

> > are formatted.  Please don't introduce more inconsistencies.

> 

> These are all from generated code, introduced by Automake: the source

> has no trailing whitespace.


It was indentation with spaces that I particularly noticed.  eg. this
one from _bfd_elf_assign_file_positions_for_non_load

-		  const char *name = sec->name;
+                  const char *name = sec->name;

-- 
Alan Modra
Australia Development Lab, IBM
Nick Alcock Sept. 25, 2019, 3:13 p.m. | #4
On 25 Sep 2019, Alan Modra uttered the following:

> On Wed, Sep 25, 2019 at 02:59:43PM +0100, Nick Alcock wrote:

>> On 25 Sep 2019, Alan Modra uttered the following:

>> 

>> > So I applied this series and the first thing I noticed was a whole lot

>> > of "git am" complaints.  In my .git/config I have

>> > [core]

>> > ...

>> >         whitespace = indent-with-non-tab,space-before-tab,trailing-space

>> > to help keep my own patches consistent with how most binutils files

>> > are formatted.  Please don't introduce more inconsistencies.

>> 

>> These are all from generated code, introduced by Automake: the source

>> has no trailing whitespace.

>

> It was indentation with spaces that I particularly noticed.  eg. this

> one from _bfd_elf_assign_file_positions_for_non_load


Oh! I quite failed to notice all of those (obviously), probably since
git diff didn't highlight them.

... hmm, ok, I think I have a regex that'll trap them, and indentation
with embedded spaces too (which the whitespace thing above would have
caught, but which was not the only problem here):

grep -E '^+(\t? +\t)|([ \t]*( ){8,})' *.patch

I'll fix everything that points out. Quite a lot has crept in, so some
of it will be in a patch stuck on the end of this series.
Jose E. Marchesi Oct. 3, 2019, 4:33 p.m. | #5
[...]
    With these comments addressed (minus --disable-libctf if you like) the
    patch series looks good to go.  In reviewing, I admit I have only
    given a quick glance over the libctf code, but that's your baby as far
    as I'm concerned.

I just pushed the corrected patch series in Nick's behalf.