[RFC/patch for stage1] Embed compiler dumps into generated .o files (was Re: Obscure crashes due to gcc 4.9 -O2 => -fisolate-erroneous-paths-dereference)

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2015-02-26 at 11:17 -0500, David Malcolm wrote:
> On Fri, 2015-02-20 at 10:29 -0700, Jeff Law wrote:
> > On 02/19/15 14:56, Chris Johns wrote:
> > > On 20/02/2015 8:23 am, Joel Sherrill wrote:
> > >>
> > >> On 2/19/2015 2:56 PM, Sandra Loosemore wrote:
> > >>> Jakub Jelinek wrote:
> > >>>> On Wed, Feb 18, 2015 at 11:21:56AM -0800, Jeff Prothero wrote:
> > >>>>> Starting with gcc 4.9, -O2 implicitly invokes
> > >>>>>
> > >>>>>      -fisolate-erroneous-paths-dereference:
> > >>>>>
> > >>>>> which
> > >>>>>
> > >>>>>      https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> > >>>>>
> > >>>>> documents as
> > >>>>>
> > >>>>>      Detect paths that trigger erroneous or undefined behavior due to
> > >>>>>      dereferencing a null pointer. Isolate those paths from the
> > >>>>> main control
> > >>>>>      flow and turn the statement with erroneous or undefined
> > >>>>> behavior into a
> > >>>>>      trap. This flag is enabled by default at -O2 and higher.
> > >>>>>
> > >>>>> This results in a sizable number of previously working embedded
> > >>>>> programs mysteriously
> > >>>>> crashing when recompiled under gcc 4.9.  The problem is that embedded
> > >>>>> programs will often have ram starting at address zero (think
> > >>>>> hardware-defined
> > >>>>> interrupt vectors, say) which gets initialized by code which the
> > >>>>> -fisolate-erroneous-paths-deference logic can recognize as reading
> > >>>>> and/or
> > >>>>> writing address zero.
> > >>>> If you have some pages mapped at address 0, you really should
> > >>>> compile your
> > >>>> code with -fno-delete-null-pointer-checks, otherwise you can run
> > >>>> into tons
> > >>>> of other issues.
> > >>> Hmmmm,  Passing the additional option in user code would be one thing,
> > >>> but what about library code?  E.g., using memcpy (either explicitly or
> > >>> implicitly for a structure copy)?
> > >>>
> > >>> It looks to me like cr16 and avr are currently the only architectures
> > >>> that disable flag_delete_null_pointer_checks entirely, but I am sure
> > >>> that this issue affects other embedded targets besides nios2, too.  E.g.
> > >>> scanning Mentor's ARM board support library, I see a whole pile of
> > >>> devices that have memory mapped at address zero (TI Stellaris/Tiva,
> > >>> Energy Micro EFM32Gxxx,  Atmel AT91SAMxxx, ....).  Plus our simulator
> > >>> BSPs assume a flat address space starting at address 0.
> > >> I forwarded this to the RTEMS list and was promptly pointed to a patch
> > >> on a Coldfire BSP where someone worked around this behavior.
> > >>
> > >> We are discussing how to deal with this. It is likely OK in user code but
> > >> horrible in BSP and driver code. We don't have a solution ourselves. We
> > >> just recognize it impacts a number of targets.
> > >>
> > >
> > > My main concern is not knowing the trap has been added to the code. If I
> > > could build an application and audit it somehow then I can manage it. We
> > > have a similar issue with the possible use of FP registers being used in
> > > general code (ISR save/restore trade off).
> > >
> > > Can the ELF be annotated in some GCC specific way that makes it to the
> > > final executable to flag this is happening ? We can then create tools to
> > > audit the executables.
> > Not really, for a variety of reasons.
> 
> Is information on this reaching the pass-specific dumpfile?  I don't see
> any explicit dumping in gimple-ssa-isolate-paths.c, but I guess that
> insert_trap_and_remove_trailing_statements could log itself to the
> dumpfile, or use the statistics framework (which itself also reaches a
> dumpfile).
> 
> Assuming the info is reaching a dumpfile, could gcc have an option to
> write its dumpfiles into a special ELF section in the .s, rather than to
> disk?
> 
> Then (given a suitable new option to e.g. eu-readelf) you'd be able to
> read the dumpfiles from a .o file, and (handwaving about linkage) from
> an execuable or library.
> 
> Not that I'm volunteering...

Perhaps foolishly I had a go at prototyping this; attached is a
proof-of-concept patch (albeit with FIXMEs and no ChangeLog or testsuite
coverage).

When writing out the final asm, each dumpfile is read, and embedded into
its own section.  Manual review of the built .o file shows that the
dumpfiles make it into them, e.g.:

$ eu-readelf -x .note.GNU-dump.tree-switchconv smoketest.o|head

Hex dump of section [28] '.note.GNU-dump.tree-switchconv', 2698 bytes at offset 0xf021:
  0x00000000 0a3b3b20 46756e63 74696f6e 20746573 .;; Function tes
  0x00000010 745f7068 69202874 6573745f 7068692c t_phi (test_phi,
  0x00000020 2066756e 63646566 5f6e6f3d 302c2064  funcdef_no=0, d
  0x00000030 65636c5f 7569643d 31383332 2c206367 ecl_uid=1832, cg
  0x00000040 72617068 5f756964 3d302c20 73796d62 raph_uid=0, symb
  0x00000050 6f6c5f6f 72646572 3d30290a 0a746573 ol_order=0)..tes
  0x00000060 745f7068 69202869 6e742069 2c20696e t_phi (int i, in
  0x00000070 74206a29 0a7b0a20 20696e74 206b3b0a t j).{.  int k;.

(again, handwaving over what happens at link time).

There could be an analogue here with debuginfo: the sections would be
big and something you'd generally want to strip, but potentially very
useful (e.g. for finding every place across a large number of compiles
where a particular optimization happened).

> > However, the compiler can do 
> > better for warning about some of these kinds of things -- but we 
> > certainly can't guarantee we catch all of them as there are cases where 
> > the point where we determine a property (such as non-nullness) may be 
> > very different from the point where we exploit that property.
> > 
> > I did propose some patches to improve the warnings back in the 4.9 time 
> > frame, but they never got reviewed.  See BZ 16351.   We'll have to 
> > revisit them during the next open development period.
> > 
> > Jeff
> 

commit a464b717cc87ba64456ef52d7829c478e6e58e91
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Feb 27 15:52:40 2015 -0500

    Prototype of embedding dumpfiles within .s/.o

diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 743344e5..1456aca 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -302,6 +302,15 @@ get_dump_file_name (struct dump_file_info *dfi) const
   return concat (dump_base_name, dump_id, dfi->suffix, NULL);
 }
 
+void
+gcc::dump_manager::for_each_dumpfile (void (*cb) (struct dump_file_info *,
+						  void *user_data),
+				      void *user_data)
+{
+  for (unsigned i = 0; i < m_extra_dump_files_in_use; i++)
+    (*cb) (&m_extra_dump_files[i], user_data);
+}
+
 /* For a given DFI, open an alternate dump filename (which could also
    be a standard stream such as stdout/stderr). If the alternate dump
    file cannot be opened, return NULL.  */
diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
index 40b6473..ebb7eba 100644
--- a/gcc/dumpfile.h
+++ b/gcc/dumpfile.h
@@ -220,6 +220,11 @@ public:
   const char *
   dump_flag_name (int phase) const;
 
+  void
+  for_each_dumpfile (void (*cb) (struct dump_file_info *,
+				 void *user_data),
+		     void *user_data);
+
 private:
 
   int
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 99cf180..10bfb24 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -114,6 +114,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "optabs.h"
 #include "tree-chkp.h"
 #include "omp-low.h"
+#include "dumpfile.h"
 
 #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
 #include "dbxout.h"
@@ -580,6 +581,84 @@ emit_debug_global_declarations (tree *vec, int len)
   timevar_pop (TV_SYMOUT);
 }
 
+/* FIXME: this is just a copy of jit-playback.c's
+   playback::context::read_dump_file with the error-handling
+   hacked out.  */
+
+static char *
+read_dump_file (const char *path)
+{
+  char *result = NULL;
+  size_t total_sz = 0;
+  char buf[4096];
+  size_t sz;
+  FILE *f_in;
+
+  f_in = fopen (path, "r");
+  if (!f_in)
+    return NULL;
+
+  while ( (sz = fread (buf, 1, sizeof (buf), f_in)) )
+    {
+      size_t old_total_sz = total_sz;
+      total_sz += sz;
+      result = reinterpret_cast <char *> (xrealloc (result, total_sz + 1));
+      memcpy (result + old_total_sz, buf, sz);
+    }
+
+  if (!feof (f_in))
+    {
+      free (result);
+      fclose (f_in);
+      return NULL;
+    }
+
+  fclose (f_in);
+
+  if (result)
+    {
+      result[total_sz] = '\0';
+      return result;
+    }
+  else
+    return xstrdup ("");
+}
+
+/* FIXME: should have a leading comment.  */
+
+static void
+embed_dumpfiles_within_asm ()
+{
+  class callback
+  {
+  public:
+    static void fn (struct dump_file_info *dfi,
+		    void */*user_data*/)
+    {
+      char *filename;
+      char *content;
+
+      filename = g->get_dumps ()->get_dump_file_name (dfi);
+      content = read_dump_file (filename);
+
+      /* FIXME: what about error handling?  */
+      if (content)
+	{
+	  unsigned int flags = SECTION_DEBUG;
+	  /* FIXME: name of section?  */
+	  char *section_name = concat (".note.GNU-dump.", dfi->swtch, NULL);
+	  switch_to_section (get_section (section_name, flags, NULL));
+	  assemble_string (content, strlen (content));
+	  free (section_name);
+	}
+      free (filename);
+      free (content);
+    }
+  };
+
+  g->get_dumps ()->for_each_dumpfile (callback::fn, NULL);
+}
+
 /* Compile an entire translation unit.  Write a file of assembly
    output and various debugging dumps.  */
 
@@ -711,6 +790,9 @@ compile_file (void)
       targetm.asm_out.output_ident (ident_str);
     }
 
+  /* FIXME: should only be enabled if an option has been set.  */
+  embed_dumpfiles_within_asm ();
+
   /* Auto profile finalization. */
   if (flag_auto_profile)
     end_auto_profile ();