Re: [PATCH v4 00/35] CTF linking support

[Nick Alcock <nick dot alcock at oracle dot com>]

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!)