Message ID | 20190924135131.441906-1-nick.alcock@oracle.com |
---|---|
Headers | show |
Series |
|
Related | show |
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
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!)
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
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.
[...] 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.