[RFC] Change PCH "checksum"

[RFC] Change PCH "checksum"

[Richard Biener]

GCC builds are currently not reproducible because for one the checksum
we compute for PCH purposes (by genchecksum) nowaways includes checksums
of archives (since we switched from checksumming a dummy executable
to checksumming object files).  That includes dates (unless built with
-D which we don't do).

Then later we switched to do thin archives so for example libbackend.a
we checksum doesn't contain the actual code anymore...

A pragmatic approach to "fix" things would be to just checksum
gtype-desc.o which should have enough state to cover PCH dependences
if I understand the workings correctly (patch below - a single
checksum would suffice so more simplifications are possible).

Another solution working on ELF systems with build-id support is
simply forgo checksumming anything and rely on the executable
build-id instead (pat^whack below as well).

Does anybody think that just checksumming gtype-desc.o is a
degradation over the current state (which checksums thin archives)?

Thanks,
Richard.

2019-02-22  Richard Biener  <rguenther@suse.de>

	c/
	* Make-lang.in (cc1-checksum.c): Checksum only gtype-desc.o.

	cp/
	* Make-lang.in (cc1plus-checksum.c): Checksum only gtype-desc.o.

	objc/
	* Make-lang.in (cc1obj-checksum.c): Checksum only gtype-desc.o.

	objcp/
	* Make-lang.in (cc1objplus-checksum.c): Checksum only gtype-desc.o.

Index: gcc/c/Make-lang.in
===================================================================
--- gcc/c/Make-lang.in	(revision 269111)
+++ gcc/c/Make-lang.in	(working copy)
@@ -70,14 +70,13 @@ endif
 # compute checksum over all object files and the options
 # re-use the checksum from the prev-final stage so it passes
 # the bootstrap comparison and allows comparing of the cc1 binary
-cc1-checksum.c : build/genchecksum$(build_exeext) checksum-options \
-	$(C_OBJS) $(BACKEND) $(LIBDEPS) 
+cc1-checksum.c : build/genchecksum$(build_exeext) gtype-desc.o 
 	if [ -f ../stage_final ] \
 	   && cmp -s ../stage_current ../stage_final; then \
 	  cp ../prev-gcc/cc1-checksum.c cc1-checksum.c; \
 	else \
-	  build/genchecksum$(build_exeext) $(C_OBJS) $(BACKEND) $(LIBDEPS) \
-                     checksum-options > cc1-checksum.c.tmp && 		 \
+	  build/genchecksum$(build_exeext) gtype-desc.o \
+                     > cc1-checksum.c.tmp && 		 \
 	  $(srcdir)/../move-if-change cc1-checksum.c.tmp cc1-checksum.c; \
 	fi
 
Index: gcc/cp/Make-lang.in
===================================================================
--- gcc/cp/Make-lang.in	(revision 269111)
+++ gcc/cp/Make-lang.in	(working copy)
@@ -105,14 +105,13 @@ cp-warn = $(STRICT_WARN)
 # compute checksum over all object files and the options
 # re-use the checksum from the prev-final stage so it passes
 # the bootstrap comparison and allows comparing of the cc1 binary
-cc1plus-checksum.c : build/genchecksum$(build_exeext) checksum-options \
-	$(CXX_OBJS) $(BACKEND) $(LIBDEPS) 
+cc1plus-checksum.c : build/genchecksum$(build_exeext) gtype-desc.o 
 	if [ -f ../stage_final ] \
 	   && cmp -s ../stage_current ../stage_final; then \
 	   cp ../prev-gcc/cc1plus-checksum.c cc1plus-checksum.c; \
 	else \
-	  build/genchecksum$(build_exeext) $(CXX_OBJS) $(BACKEND) $(LIBDEPS) \
-                     checksum-options > cc1plus-checksum.c.tmp &&	   \
+	  build/genchecksum$(build_exeext) gtype-desc.o \
+                     > cc1plus-checksum.c.tmp &&	   \
 	  $(srcdir)/../move-if-change cc1plus-checksum.c.tmp cc1plus-checksum.c; \
 	fi
 
Index: gcc/objc/Make-lang.in
===================================================================
--- gcc/objc/Make-lang.in	(revision 269111)
+++ gcc/objc/Make-lang.in	(working copy)
@@ -56,10 +56,9 @@ OBJC_OBJS = objc/objc-lang.o objc/objc-a
 
 objc_OBJS = $(OBJC_OBJS) cc1obj-checksum.o
 
-cc1obj-checksum.c : build/genchecksum$(build_exeext) checksum-options \
-        $(OBJC_OBJS) $(C_AND_OBJC_OBJS) $(BACKEND) $(LIBDEPS)
-	build/genchecksum$(build_exeext) $(OBJC_OBJS) $(C_AND_OBJC_OBJS) \
-        $(BACKEND) $(LIBDEPS) checksum-options > cc1obj-checksum.c.tmp && \
+cc1obj-checksum.c : build/genchecksum$(build_exeext) gtype-desc.o
+	build/genchecksum$(build_exeext) gtype-desc.o
+		> cc1obj-checksum.c.tmp && \
 	$(srcdir)/../move-if-change cc1obj-checksum.c.tmp cc1obj-checksum.c
 
 cc1obj$(exeext): $(OBJC_OBJS) $(C_AND_OBJC_OBJS) cc1obj-checksum.o $(BACKEND) $(LIBDEPS)
Index: gcc/objcp/Make-lang.in
===================================================================
--- gcc/objcp/Make-lang.in	(revision 269111)
+++ gcc/objcp/Make-lang.in	(working copy)
@@ -59,10 +59,9 @@ OBJCXX_OBJS = objcp/objcp-act.o objcp/ob
 
 obj-c++_OBJS = $(OBJCXX_OBJS) cc1objplus-checksum.o
 
-cc1objplus-checksum.c : build/genchecksum$(build_exeext) checksum-options \
-	$(OBJCXX_OBJS) $(BACKEND) $(LIBDEPS)
-	build/genchecksum$(build_exeext) $(OBJCXX_OBJS) $(BACKEND) \
-		$(LIBDEPS) checksum-options > cc1objplus-checksum.c.tmp && \
+cc1objplus-checksum.c : build/genchecksum$(build_exeext) gtype-desc.o 
+	build/genchecksum$(build_exeext) gtype-desc.o
+		> cc1objplus-checksum.c.tmp && \
 	$(srcdir)/../move-if-change cc1objplus-checksum.c.tmp \
 	cc1objplus-checksum.c
 


2019-02-22  Richard Biener  <rguenther@suse.de>

	* ...

Index: gcc/c-family/c-pch.c
===================================================================
--- gcc/c-family/c-pch.c	(revision 269111)
+++ gcc/c-family/c-pch.c	(working copy)
@@ -45,10 +45,6 @@ enum {
   MATCH_SIZE = ARRAY_SIZE (pch_matching)
 };
 
-/* The value of the checksum in the dummy compiler that is actually
-   checksummed.  That compiler should never be run.  */
-static const char no_checksum[16] = { 0 };
-
 /* Information about flags and suchlike that affect PCH validity.
 
    Before this structure is read, both an initial 8-character identification
@@ -69,6 +65,67 @@ static FILE *pch_outfile;
 
 static const char *get_ident (void);
 
+#if _GNU_SOURCE
+#include <link.h>
+
+#define ALIGN(val, align)      (((val) + (align) - 1) & ~((align) - 1))
+
+struct build_id_note {
+    /* The NHdr.  */
+    uint32_t namesz;
+    uint32_t descsz;
+    uint32_t type;
+
+    char name[4]; /* Note name for build-id is "GNU\0" */
+    unsigned char build_id[16];
+};
+
+static int
+get_build_id_1 (struct dl_phdr_info *info, size_t, void *data)
+{
+  for (unsigned i = 0; i < info->dlpi_phnum; ++i)
+    {
+      if (info->dlpi_phdr[i].p_type != PT_NOTE)
+	continue;
+      build_id_note *note
+	= (build_id_note *)(info->dlpi_addr + info->dlpi_phdr[i].p_vaddr);
+      ptrdiff_t size = info->dlpi_phdr[i].p_filesz;
+      while (size >= (ptrdiff_t)sizeof (build_id_note))
+	{
+	  if (note->type == NT_GNU_BUILD_ID
+	      && note->namesz == 4
+	      && note->descsz >= 16)
+	    {
+	      memcpy (data, note->build_id, 16);
+	      return 1;
+	    }
+	  size_t offset = (sizeof (uint32_t) * 3
+			   + ALIGN(note->namesz, 4)
+			   + ALIGN(note->descsz, 4));
+	  note = (build_id_note *)((char *)note + offset);
+	  size -= offset;
+	}
+    }
+
+  return 0;
+}
+
+static const unsigned char *
+get_build_id ()
+{
+  static unsigned char build_id[16];
+  if (!dl_iterate_phdr (get_build_id_1, build_id))
+    return NULL;
+  return build_id;
+}
+#else
+static const unsigned char *
+get_build_id ()
+{
+  return NULL;
+}
+#endif
+
 /* Compute an appropriate 8-byte magic number for the PCH file, so that
    utilities like file(1) can identify it, and so that GCC can quickly
    ignore non-PCH files and PCH files that are of a completely different
@@ -111,8 +168,6 @@ pch_init (void)
 		 pch_file);
   pch_outfile = f;
 
-  gcc_assert (memcmp (executable_checksum, no_checksum, 16) != 0);
-
   memset (&v, '\0', sizeof (v));
   v.debug_info_type = write_symbols;
   {
@@ -126,8 +181,11 @@ pch_init (void)
   v.pch_init = &pch_init;
   target_validity = targetm.get_pch_validity (&v.target_data_length);
 
+  const unsigned char *chksum = get_build_id ();
+  if (!chksum)
+    chksum = executable_checksum;
   if (fwrite (partial_pch, IDENT_LENGTH, 1, f) != 1
-      || fwrite (executable_checksum, 16, 1, f) != 1
+      || fwrite (chksum, 16, 1, f) != 1
       || fwrite (&v, sizeof (v), 1, f) != 1
       || fwrite (target_validity, v.target_data_length, 1, f) != 1)
     fatal_error (input_location, "can%'t write to %s: %m", pch_file);
@@ -212,8 +270,6 @@ c_common_valid_pch (cpp_reader *pfile, c
   /* Perform a quick test of whether this is a valid
      precompiled header for the current language.  */
 
-  gcc_assert (memcmp (executable_checksum, no_checksum, 16) != 0);
-
   sizeread = read (fd, ident, IDENT_LENGTH + 16);
   if (sizeread == -1)
     fatal_error (input_location, "can%'t read %s: %m", name);
@@ -245,7 +301,10 @@ c_common_valid_pch (cpp_reader *pfile, c
 	}
       return 2;
     }
-  if (memcmp (ident + IDENT_LENGTH, executable_checksum, 16) != 0)
+  const unsigned char *chksum = get_build_id ();
+  if (!chksum)
+    chksum = executable_checksum;
+  if (memcmp (ident + IDENT_LENGTH, chksum, 16) != 0)
     {
       if (cpp_get_options (pfile)->warn_invalid_pch)
 	cpp_error (pfile, CPP_DL_WARNING,
Index: gcc/genchecksum.c
===================================================================
--- gcc/genchecksum.c	(revision 269111)
+++ gcc/genchecksum.c	(working copy)
@@ -113,8 +113,13 @@ main (int argc, char ** argv)
   puts ("#include \"config.h\"");
   puts ("#include \"system.h\"");
   fputs ("EXPORTED_CONST unsigned char executable_checksum[16] = { ", stdout);
+#if _GNU_SOURCE
+  for (i = 0; i < 16; i++)
+    printf ("0x%02x%s", 0, i == 15 ? " };\n" : ", ");
+#else
   for (i = 0; i < 16; i++)
     printf ("0x%02x%s", result[i], i == 15 ? " };\n" : ", ");
+#endif
 
   return 0;
 }

Re: [RFC] Change PCH "checksum"

[Jeff Law]

On 2/22/19 4:29 AM, Richard Biener wrote:
> 
> GCC builds are currently not reproducible because for one the checksum
> we compute for PCH purposes (by genchecksum) nowaways includes checksums
> of archives (since we switched from checksumming a dummy executable
> to checksumming object files).  That includes dates (unless built with
> -D which we don't do).
> 
> Then later we switched to do thin archives so for example libbackend.a
> we checksum doesn't contain the actual code anymore...
> 
> A pragmatic approach to "fix" things would be to just checksum
> gtype-desc.o which should have enough state to cover PCH dependences
> if I understand the workings correctly (patch below - a single
> checksum would suffice so more simplifications are possible).
> 
> Another solution working on ELF systems with build-id support is
> simply forgo checksumming anything and rely on the executable
> build-id instead (pat^whack below as well).
> 
> Does anybody think that just checksumming gtype-desc.o is a
> degradation over the current state (which checksums thin archives)?
> 
> Thanks,
> Richard.
> 
> 2019-02-22  Richard Biener  <rguenther@suse.de>
> 
> 	c/
> 	* Make-lang.in (cc1-checksum.c): Checksum only gtype-desc.o.
> 
> 	cp/
> 	* Make-lang.in (cc1plus-checksum.c): Checksum only gtype-desc.o.
> 
> 	objc/
> 	* Make-lang.in (cc1obj-checksum.c): Checksum only gtype-desc.o.
> 
> 	objcp/
> 	* Make-lang.in (cc1objplus-checksum.c): Checksum only gtype-desc.o.
ISTM that gtype-desc effectively describes the structure of all the GC data.

Given we're summing the thin-archives, we're already missing things like
a change in static data.  So I don't think your patch is a degradation
over the current state.  I'm not 100% sure the current state is correct
though :-)

jeff

Re: [RFC] Change PCH "checksum"

[Jakub Jelinek]

On Fri, Feb 22, 2019 at 08:47:09AM -0700, Jeff Law wrote:
> > 2019-02-22  Richard Biener  <rguenther@suse.de>
> > 
> > 	c/
> > 	* Make-lang.in (cc1-checksum.c): Checksum only gtype-desc.o.
> > 
> > 	cp/
> > 	* Make-lang.in (cc1plus-checksum.c): Checksum only gtype-desc.o.
> > 
> > 	objc/
> > 	* Make-lang.in (cc1obj-checksum.c): Checksum only gtype-desc.o.
> > 
> > 	objcp/
> > 	* Make-lang.in (cc1objplus-checksum.c): Checksum only gtype-desc.o.
> ISTM that gtype-desc effectively describes the structure of all the GC data.
> 
> Given we're summing the thin-archives, we're already missing things like
> a change in static data.  So I don't think your patch is a degradation
> over the current state.  I'm not 100% sure the current state is correct
> though :-)

Does it cover everything though?  I believe gtype-desc.c only covers a small
portion, the rest is in all the gtype-*.h and gt-*.h headers that are
included in the various object files.
So, either we need to checksum all the object files that include gt-*.h or
gtype-*.h headers in addition to gtype-desc.o, or perhaps checksum
gtype.state ?  Though, that state wouldn't cover changes in ABI etc.

	Jakub

Re: [RFC] Change PCH "checksum"

[Richard Biener]

On February 22, 2019 5:03:46 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Fri, Feb 22, 2019 at 08:47:09AM -0700, Jeff Law wrote:
>> > 2019-02-22  Richard Biener  <rguenther@suse.de>
>> > 
>> > 	c/
>> > 	* Make-lang.in (cc1-checksum.c): Checksum only gtype-desc.o.
>> > 
>> > 	cp/
>> > 	* Make-lang.in (cc1plus-checksum.c): Checksum only gtype-desc.o.
>> > 
>> > 	objc/
>> > 	* Make-lang.in (cc1obj-checksum.c): Checksum only gtype-desc.o.
>> > 
>> > 	objcp/
>> > 	* Make-lang.in (cc1objplus-checksum.c): Checksum only
>gtype-desc.o.
>> ISTM that gtype-desc effectively describes the structure of all the
>GC data.
>> 
>> Given we're summing the thin-archives, we're already missing things
>like
>> a change in static data.  So I don't think your patch is a
>degradation
>> over the current state.  I'm not 100% sure the current state is
>correct
>> though :-)
>
>Does it cover everything though?  I believe gtype-desc.c only covers a
>small
>portion, the rest is in all the gtype-*.h and gt-*.h headers that are
>included in the various object files.
>So, either we need to checksum all the object files that include gt-*.h
>or
>gtype-*.h headers in addition to gtype-desc.o, or perhaps checksum
>gtype.state ?  Though, that state wouldn't cover changes in ABI etc.

Gtype-desc.o does not cover everything indeed. But the current state doesn't cover Gtype-desc.o... Slightly better would be to Re-include frontend objects. 

Not sure why we checksummed build flags for example. Isn't it enough to handle gty walking changes?

Anyway, for suse I'm probably using the build-id thing. 

Richard. 

>	Jakub

Re: [RFC] Change PCH "checksum"

[Mark Wielaard]

On Fri, 2019-02-22 at 12:29 +0100, Richard Biener wrote:
> +struct build_id_note {
> +    /* The NHdr.  */
> +    uint32_t namesz;
> +    uint32_t descsz;
> +    uint32_t type;
> +
> +    char name[4]; /* Note name for build-id is "GNU\0" */
> +    unsigned char build_id[16];
> +};

Note that build-ids can be of different sizes depending on the style
used to generate them, you get the correct size by looking at the
descsz.

> +static int
> +get_build_id_1 (struct dl_phdr_info *info, size_t, void *data)
> +{
> +  for (unsigned i = 0; i < info->dlpi_phnum; ++i)
> +    {
> +      if (info->dlpi_phdr[i].p_type != PT_NOTE)
> +	continue;
> +      build_id_note *note
> +	= (build_id_note *)(info->dlpi_addr + info->dlpi_phdr[i].p_vaddr);
> +      ptrdiff_t size = info->dlpi_phdr[i].p_filesz;
> +      while (size >= (ptrdiff_t)sizeof (build_id_note))
> +	{
> +	  if (note->type == NT_GNU_BUILD_ID
> +	      && note->namesz == 4
> +	      && note->descsz >= 16)
> +	    {
> +	      memcpy (data, note->build_id, 16);
> +	      return 1;
> +	    }
> +	  size_t offset = (sizeof (uint32_t) * 3
> +			   + ALIGN(note->namesz, 4)
> +			   + ALIGN(note->descsz, 4));
> +	  note = (build_id_note *)((char *)note + offset);
> +	  size -= offset;

Since the introduction of GNU Property notes this is (sadly) no longer
the correct way to iterate through ELF notes. The padding of names and
desc  might now depend on the alignment of the PT_NOTE segment.
https://sourceware.org/ml/binutils/2018-09/msg00359.html

Cheers,

Mark

Re: [RFC] Change PCH "checksum"

[Richard Biener]

On Mon, 25 Feb 2019, Mark Wielaard wrote:

> On Fri, 2019-02-22 at 12:29 +0100, Richard Biener wrote:
> > +struct build_id_note {
> > +    /* The NHdr.  */
> > +    uint32_t namesz;
> > +    uint32_t descsz;
> > +    uint32_t type;
> > +
> > +    char name[4]; /* Note name for build-id is "GNU\0" */
> > +    unsigned char build_id[16];
> > +};
> 
> Note that build-ids can be of different sizes depending on the style
> used to generate them, you get the correct size by looking at the
> descsz.

Yeah, as said it's currently a hack...

> > +static int
> > +get_build_id_1 (struct dl_phdr_info *info, size_t, void *data)
> > +{
> > +  for (unsigned i = 0; i < info->dlpi_phnum; ++i)
> > +    {
> > +      if (info->dlpi_phdr[i].p_type != PT_NOTE)
> > +	continue;
> > +      build_id_note *note
> > +	= (build_id_note *)(info->dlpi_addr + info->dlpi_phdr[i].p_vaddr);
> > +      ptrdiff_t size = info->dlpi_phdr[i].p_filesz;
> > +      while (size >= (ptrdiff_t)sizeof (build_id_note))
> > +	{
> > +	  if (note->type == NT_GNU_BUILD_ID
> > +	      && note->namesz == 4
> > +	      && note->descsz >= 16)
> > +	    {
> > +	      memcpy (data, note->build_id, 16);
> > +	      return 1;
> > +	    }
> > +	  size_t offset = (sizeof (uint32_t) * 3
> > +			   + ALIGN(note->namesz, 4)
> > +			   + ALIGN(note->descsz, 4));
> > +	  note = (build_id_note *)((char *)note + offset);
> > +	  size -= offset;
> 
> Since the introduction of GNU Property notes this is (sadly) no longer
> the correct way to iterate through ELF notes. The padding of names and
> desc  might now depend on the alignment of the PT_NOTE segment.
> https://sourceware.org/ml/binutils/2018-09/msg00359.html

Ick, that's of course worse ;)  So it's not entirely clear what
the correct thing to do is - from how I read the mail at the above
link only iff sh_align of the note section is exactly 8 the above
ALIGN would use 8 byte alignment and else 4 is correct (independent
on sh_align).  Or can I assume sh_align of the note section is
"correct" for all existing binaries?  Note also the eventual difference
between note sections and note program headers which have another,
possibly different(?) alignment?  It's of course "easy" to replace
4 above by info->dlpi_phdr[i].p_align (but the align field differs
in width between elfclass 32 and 64 ... :/).

So - is merely changing the re-alignment from 4 to 
info->dlpi_phdr[i].p_align "correct"?

Richard.

> Cheers,
> 
> Mark
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [RFC] Change PCH "checksum"

[Mark Wielaard]

On Tue, 2019-02-26 at 09:33 +0100, Richard Biener wrote:
> On Mon, 25 Feb 2019, Mark Wielaard wrote:
> > Since the introduction of GNU Property notes this is (sadly) no
> > longer
> > the correct way to iterate through ELF notes. The padding of names
> > and
> > desc  might now depend on the alignment of the PT_NOTE segment.
> > https://sourceware.org/ml/binutils/2018-09/msg00359.html
> 
> Ick, that's of course worse ;)  So it's not entirely clear what
> the correct thing to do is - from how I read the mail at the above
> link only iff sh_align of the note section is exactly 8 the above
> ALIGN would use 8 byte alignment and else 4 is correct (independent
> on sh_align).  Or can I assume sh_align of the note section is
> "correct" for all existing binaries?  Note also the eventual
> difference
> between note sections and note program headers which have another,
> possibly different(?) alignment?  It's of course "easy" to replace
> 4 above by info->dlpi_phdr[i].p_align (but the align field differs
> in width between elfclass 32 and 64 ... :/).
> 
> So - is merely changing the re-alignment from 4 to 
> info->dlpi_phdr[i].p_align "correct"?

Yes, you will have multiple note segments one that combines the 4
padded notes and one that combines the 8 padded notes.
Some tools put 0 or 1 in the align field, so you might want to use
(completely untested):
align = (p_align <= 4) ? 4 : 8;
offset += ALIGN ((ALIGN (sizeof (uint32_t) * 3 + namesz, align)
                  + descsz), align);

Cheers,

Mark

Re: [RFC] Change PCH "checksum"

[Richard Biener]

On Tue, 26 Feb 2019, Mark Wielaard wrote:

> On Tue, 2019-02-26 at 09:33 +0100, Richard Biener wrote:
> > On Mon, 25 Feb 2019, Mark Wielaard wrote:
> > > Since the introduction of GNU Property notes this is (sadly) no
> > > longer
> > > the correct way to iterate through ELF notes. The padding of names
> > > and
> > > desc  might now depend on the alignment of the PT_NOTE segment.
> > > https://sourceware.org/ml/binutils/2018-09/msg00359.html
> > 
> > Ick, that's of course worse ;)  So it's not entirely clear what
> > the correct thing to do is - from how I read the mail at the above
> > link only iff sh_align of the note section is exactly 8 the above
> > ALIGN would use 8 byte alignment and else 4 is correct (independent
> > on sh_align).  Or can I assume sh_align of the note section is
> > "correct" for all existing binaries?  Note also the eventual
> > difference
> > between note sections and note program headers which have another,
> > possibly different(?) alignment?  It's of course "easy" to replace
> > 4 above by info->dlpi_phdr[i].p_align (but the align field differs
> > in width between elfclass 32 and 64 ... :/).
> > 
> > So - is merely changing the re-alignment from 4 to 
> > info->dlpi_phdr[i].p_align "correct"?
> 
> Yes, you will have multiple note segments one that combines the 4
> padded notes and one that combines the 8 padded notes.
> Some tools put 0 or 1 in the align field, so you might want to use
> (completely untested):
> align = (p_align <= 4) ? 4 : 8;
> offset += ALIGN ((ALIGN (sizeof (uint32_t) * 3 + namesz, align)
>                   + descsz), align);

That would mean when p_align == 8 the note name isn't 8-aligned
but just 4-aligned?  That is, sizeof (Elf*_Nhdr) == 12, and the
name starts right after that instead of being aligned according
to p_align?  That sounds odd...  So p_align only applies to
the descriptor?

Richard.

> Cheers,
> 
> Mark
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [RFC] Change PCH "checksum"

[Richard Biener]

On Tue, 26 Feb 2019, Richard Biener wrote:

> On Tue, 26 Feb 2019, Mark Wielaard wrote:
> 
> > On Tue, 2019-02-26 at 09:33 +0100, Richard Biener wrote:
> > > On Mon, 25 Feb 2019, Mark Wielaard wrote:
> > > > Since the introduction of GNU Property notes this is (sadly) no
> > > > longer
> > > > the correct way to iterate through ELF notes. The padding of names
> > > > and
> > > > desc  might now depend on the alignment of the PT_NOTE segment.
> > > > https://sourceware.org/ml/binutils/2018-09/msg00359.html
> > > 
> > > Ick, that's of course worse ;)  So it's not entirely clear what
> > > the correct thing to do is - from how I read the mail at the above
> > > link only iff sh_align of the note section is exactly 8 the above
> > > ALIGN would use 8 byte alignment and else 4 is correct (independent
> > > on sh_align).  Or can I assume sh_align of the note section is
> > > "correct" for all existing binaries?  Note also the eventual
> > > difference
> > > between note sections and note program headers which have another,
> > > possibly different(?) alignment?  It's of course "easy" to replace
> > > 4 above by info->dlpi_phdr[i].p_align (but the align field differs
> > > in width between elfclass 32 and 64 ... :/).
> > > 
> > > So - is merely changing the re-alignment from 4 to 
> > > info->dlpi_phdr[i].p_align "correct"?
> > 
> > Yes, you will have multiple note segments one that combines the 4
> > padded notes and one that combines the 8 padded notes.
> > Some tools put 0 or 1 in the align field, so you might want to use
> > (completely untested):
> > align = (p_align <= 4) ? 4 : 8;
> > offset += ALIGN ((ALIGN (sizeof (uint32_t) * 3 + namesz, align)
> >                   + descsz), align);
> 
> That would mean when p_align == 8 the note name isn't 8-aligned
> but just 4-aligned?  That is, sizeof (Elf*_Nhdr) == 12, and the
> name starts right after that instead of being aligned according
> to p_align?  That sounds odd...  So p_align only applies to
> the descriptor?

So rather like the following (simplified for _GNU_SOURCE since
link.h includes elf.h there).  I've not yet come along binaries
with different p_align so I can't really test it.

#if _GNU_SOURCE
#include <link.h>

#define ALIGN(val, align)      (((val) + (align) - 1) & ~((align) - 1))

static int
get_build_id_1 (struct dl_phdr_info *info, size_t, void *data)
{ 
  for (unsigned i = 0; i < info->dlpi_phnum; ++i)
    { 
      if (info->dlpi_phdr[i].p_type != PT_NOTE)
        continue;
      ElfW(Nhdr) *nhdr
        = (ElfW(Nhdr) *)(info->dlpi_addr + info->dlpi_phdr[i].p_vaddr);
      ptrdiff_t size = info->dlpi_phdr[i].p_filesz;
      ptrdiff_t align = info->dlpi_phdr[i].p_align;
      if (align < 4)
        align = 4;
      while (size >= (ptrdiff_t)sizeof (ElfW(Nhdr)))
        { 
          if (nhdr->n_type == NT_GNU_BUILD_ID
              && nhdr->n_namesz == 4
              && strncmp ((char *)nhdr
                          + ALIGN (sizeof (ElfW(Nhdr)), align),
                          "GNU", 4) == 0
              && nhdr->n_descsz >= 16)
            { 
              memcpy (data,
                      (char *)nhdr
                      + ALIGN (sizeof (ElfW(Nhdr)), align)
                      + ALIGN (nhdr->n_namesz, align), 16);
              return 1;
            }
          size_t offset = (ALIGN (sizeof (ElfW(Nhdr)), align)
                           + ALIGN(nhdr->n_namesz, align)
                           + ALIGN(nhdr->n_descsz, align));
          nhdr = (ElfW(Nhdr) *)((char *)nhdr + offset);
          size -= offset;
        }
    }

  return 0;
}

Re: [RFC] Change PCH "checksum"

[Michael Matz]

Hi,

On Tue, 26 Feb 2019, Richard Biener wrote:

> get_build_id_1 (struct dl_phdr_info *info, size_t, void *data)
> { 

Isn't this all a bit silly?  We could simply encode the svn revision, or 
maybe even just some random bytes generated once in stage1 at build time 
as "checksum" and be done with.  In the latter case PCHs will then not 
work across different compiler builds, but so what?


Ciao,
Michael.

Re: [RFC] Change PCH "checksum"

[Richard Biener]

On Tue, 26 Feb 2019, Michael Matz wrote:

> Hi,
> 
> On Tue, 26 Feb 2019, Richard Biener wrote:
> 
> > get_build_id_1 (struct dl_phdr_info *info, size_t, void *data)
> > { 
> 
> Isn't this all a bit silly?  We could simply encode the svn revision, or 
> maybe even just some random bytes generated once in stage1 at build time 
> as "checksum" and be done with.  In the latter case PCHs will then not 
> work across different compiler builds, but so what?

Yes, a random number would work for PCH purposes but of course not
for reproducible builds.  Somehow even compile-options are relevant
though so I'm not really sure how volatile the PCH format is.
That is, whether for example checksumming sources would work.

But yeah, I considered a --with-pch-checksum=XYZ to make this
configurable (where we could for example checksum the rpm
changes - iff PCHs of two different builds - say, one with checking
enabled and one with checking disabled - really interoperate).

Still, using the build-id looks so "obvious" ...

Richard.

Re: [RFC] Change PCH "checksum"

[Mark Wielaard]

On Tue, 2019-02-26 at 15:36 +0100, Richard Biener wrote:
> On Tue, 26 Feb 2019, Mark Wielaard wrote:
> 
> > On Tue, 2019-02-26 at 09:33 +0100, Richard Biener wrote:
> > > On Mon, 25 Feb 2019, Mark Wielaard wrote:
> > > > Since the introduction of GNU Property notes this is (sadly) no
> > > > longer
> > > > the correct way to iterate through ELF notes. The padding of
> > > > names
> > > > and
> > > > desc  might now depend on the alignment of the PT_NOTE segment.
> > > > https://sourceware.org/ml/binutils/2018-09/msg00359.html
> > > 
> > > Ick, that's of course worse ;)  So it's not entirely clear what
> > > the correct thing to do is - from how I read the mail at the
> > > above
> > > link only iff sh_align of the note section is exactly 8 the above
> > > ALIGN would use 8 byte alignment and else 4 is correct
> > > (independent
> > > on sh_align).  Or can I assume sh_align of the note section is
> > > "correct" for all existing binaries?  Note also the eventual
> > > difference
> > > between note sections and note program headers which have
> > > another,
> > > possibly different(?) alignment?  It's of course "easy" to
> > > replace
> > > 4 above by info->dlpi_phdr[i].p_align (but the align field
> > > differs
> > > in width between elfclass 32 and 64 ... :/).
> > > 
> > > So - is merely changing the re-alignment from 4 to 
> > > info->dlpi_phdr[i].p_align "correct"?
> > 
> > Yes, you will have multiple note segments one that combines the 4
> > padded notes and one that combines the 8 padded notes.
> > Some tools put 0 or 1 in the align field, so you might want to use
> > (completely untested):
> > align = (p_align <= 4) ? 4 : 8;
> > offset += ALIGN ((ALIGN (sizeof (uint32_t) * 3 + namesz, align)
> >                   + descsz), align);
> 
> That would mean when p_align == 8 the note name isn't 8-aligned
> but just 4-aligned?  That is, sizeof (Elf*_Nhdr) == 12, and the
> name starts right after that instead of being aligned according
> to p_align?  That sounds odd...  So p_align only applies to
> the descriptor?

Yes, it is that odd. There are 3 kinds of ELF notes.

The traditional ones as used by GNU and Solaris, which use 4 byte words
for everything whether in ELFCLASS32 or ELFCLASS64 and which are 4 byte
aligned themselves.

The gabi ones, which are similar for ELFCLASS32 but for ELFCLASS64 all
words are 8 bytes and 8 bytes aligned themselves (as used by HPUX).

And the new style GNU Property notes, only used in ELFCLASS64, which
use 4 byte words for the first 3 fields, immediately followed by the
name bytes, padded so that desc is 8 bytes aligned and the note as a
whole is 8 byte aligned.

Cheers,

Mark

Re: [RFC] Change PCH "checksum"

[Richard Biener]

On Tue, 26 Feb 2019, Mark Wielaard wrote:

> On Tue, 2019-02-26 at 15:36 +0100, Richard Biener wrote:
> > On Tue, 26 Feb 2019, Mark Wielaard wrote:
> > 
> > > On Tue, 2019-02-26 at 09:33 +0100, Richard Biener wrote:
> > > > On Mon, 25 Feb 2019, Mark Wielaard wrote:
> > > > > Since the introduction of GNU Property notes this is (sadly) no
> > > > > longer
> > > > > the correct way to iterate through ELF notes. The padding of
> > > > > names
> > > > > and
> > > > > desc  might now depend on the alignment of the PT_NOTE segment.
> > > > > https://sourceware.org/ml/binutils/2018-09/msg00359.html
> > > > 
> > > > Ick, that's of course worse ;)  So it's not entirely clear what
> > > > the correct thing to do is - from how I read the mail at the
> > > > above
> > > > link only iff sh_align of the note section is exactly 8 the above
> > > > ALIGN would use 8 byte alignment and else 4 is correct
> > > > (independent
> > > > on sh_align).  Or can I assume sh_align of the note section is
> > > > "correct" for all existing binaries?  Note also the eventual
> > > > difference
> > > > between note sections and note program headers which have
> > > > another,
> > > > possibly different(?) alignment?  It's of course "easy" to
> > > > replace
> > > > 4 above by info->dlpi_phdr[i].p_align (but the align field
> > > > differs
> > > > in width between elfclass 32 and 64 ... :/).
> > > > 
> > > > So - is merely changing the re-alignment from 4 to 
> > > > info->dlpi_phdr[i].p_align "correct"?
> > > 
> > > Yes, you will have multiple note segments one that combines the 4
> > > padded notes and one that combines the 8 padded notes.
> > > Some tools put 0 or 1 in the align field, so you might want to use
> > > (completely untested):
> > > align = (p_align <= 4) ? 4 : 8;
> > > offset += ALIGN ((ALIGN (sizeof (uint32_t) * 3 + namesz, align)
> > >                   + descsz), align);
> > 
> > That would mean when p_align == 8 the note name isn't 8-aligned
> > but just 4-aligned?  That is, sizeof (Elf*_Nhdr) == 12, and the
> > name starts right after that instead of being aligned according
> > to p_align?  That sounds odd...  So p_align only applies to
> > the descriptor?
> 
> Yes, it is that odd. There are 3 kinds of ELF notes.
> 
> The traditional ones as used by GNU and Solaris, which use 4 byte words
> for everything whether in ELFCLASS32 or ELFCLASS64 and which are 4 byte
> aligned themselves.
> 
> The gabi ones, which are similar for ELFCLASS32 but for ELFCLASS64 all
> words are 8 bytes and 8 bytes aligned themselves (as used by HPUX).
> 
> And the new style GNU Property notes, only used in ELFCLASS64, which
> use 4 byte words for the first 3 fields, immediately followed by the
> name bytes, padded so that desc is 8 bytes aligned and the note as a
> whole is 8 byte aligned.

I wonder how to distinguish the latter two - does one really need
to test the size of ElfW(Nhdr).n_namesz for example?  Why was the
GNU Property one chosen this way?!  Is the first case (traditional
GNU note) with p_align == 8 invalid?  That is, is testing p_align
really the correct way to determine how the individual parts are
aligned?  I guess not.

So - how do I identify a GNU Property note vs. a traditional
note vs. a gabi one?

Why was the third one added?! (I guess I asked that already...)

Richard.

> Cheers,
> 
> Mark
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [RFC] Change PCH "checksum"

[Mark Wielaard]

On Tue, 2019-02-26 at 18:13 +0100, Richard Biener wrote:
> > > That would mean when p_align == 8 the note name isn't 8-aligned
> > > but just 4-aligned?  That is, sizeof (Elf*_Nhdr) == 12, and the
> > > name starts right after that instead of being aligned according
> > > to p_align?  That sounds odd...  So p_align only applies to
> > > the descriptor?
> > 
> > Yes, it is that odd. There are 3 kinds of ELF notes.
> > 
> > The traditional ones as used by GNU and Solaris, which use 4 byte
> > words
> > for everything whether in ELFCLASS32 or ELFCLASS64 and which are 4
> > byte
> > aligned themselves.
> > 
> > The gabi ones, which are similar for ELFCLASS32 but for ELFCLASS64
> > all
> > words are 8 bytes and 8 bytes aligned themselves (as used by HPUX).
> > 
> > And the new style GNU Property notes, only used in ELFCLASS64,
> > which
> > use 4 byte words for the first 3 fields, immediately followed by
> > the
> > name bytes, padded so that desc is 8 bytes aligned and the note as
> > a
> > whole is 8 byte aligned.
> 
> I wonder how to distinguish the latter two - does one really need
> to test the size of ElfW(Nhdr).n_namesz for example?

I think the second one is only used on HPUX.
Everything else uses the 4 byte words variant.
I have only encountered the traditional note types and the new GNU
Properties notes (on Fedora, I don't believe any other distro has,
yet?, adopted them).

>   Why was the GNU Property one chosen this way?!

All I can do is point you at the "consensus" document:
https://sourceware.org/ml/binutils/2018-09/msg00282.html
and the replies to that.

>   Is the first case (traditional
> GNU note) with p_align == 8 invalid?

Yes, I believe so.

>   That is, is testing p_align
> really the correct way to determine how the individual parts are
> aligned?  I guess not.

I do think that is the only way. If the PT_NOTE segment or SHT_NOTE
segment has an alignment of 8 then it is a GNU Properties note with the
new layout (at least on GNU systems). Cary did propose some additional
constraints which might be helpful:
https://sourceware.org/ml/binutils/2018-09/msg00359.html

> So - how do I identify a GNU Property note vs. a traditional
> note vs. a gabi one?
> 
> Why was the third one added?! (I guess I asked that already...)

Yeah... See above.

Cheers,

Mark

Re: [RFC] Change PCH "checksum"

[Florian Weimer]

* Richard Biener:

>> Since the introduction of GNU Property notes this is (sadly) no longer
>> the correct way to iterate through ELF notes. The padding of names and
>> desc  might now depend on the alignment of the PT_NOTE segment.
>> https://sourceware.org/ml/binutils/2018-09/msg00359.html
>
> Ick, that's of course worse ;)  So it's not entirely clear what
> the correct thing to do is - from how I read the mail at the above
> link only iff sh_align of the note section is exactly 8 the above
> ALIGN would use 8 byte alignment and else 4 is correct (independent
> on sh_align).  Or can I assume sh_align of the note section is
> "correct" for all existing binaries?

sh_align doesn't come into play if you look at program headers.

A GNU build ID note will not end up in a PT_NOTE segment with 8-byte
alignment, so I think you can skip those early, like this:

+      if (info->dlpi_phdr[i].p_type != PT_NOTE
+          || info->dlpi_phdr[i].p_align != 4)
+	 continue;

(Untested, of course.)

Thanks,
Florian

Re: [RFC] Change PCH "checksum"

[Nathan Sidwell]

On 2/26/19 11:18 AM, Michael Matz wrote:
> Hi,
> 
> On Tue, 26 Feb 2019, Richard Biener wrote:
> 
>> get_build_id_1 (struct dl_phdr_info *info, size_t, void *data)
>> {
> 
> Isn't this all a bit silly?  We could simply encode the svn revision, or
> maybe even just some random bytes generated once in stage1 at build time
> as "checksum" and be done with.  In the latter case PCHs will then not
> work across different compiler builds, but so what?

FWIW, here's what I do on the modules branch, which has similar 
versioning concerns.

1) in experimental mode, I stamp with the local timestamp modification 
of the cp directory.  I allow mixing versions with in the same day (with 
a warning).

2) in non-experimental mode, I plan to lock to the GCC major.minor tuple.

PCH would need to look at more than the cp directory, but that's good 
enough for me.  I'm pondering an 'ignore version' developer option, but 
as it's only me right now, that's not been a need.

nathan

-- 
Nathan Sidwell