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

[Alan Modra <amodra at gmail dot com>]

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