[PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Egeyar Bagcioglu
Hello,

I would like to propose the following patches which introduce a compile option --record-gcc-command-line. When passed to gcc, it saves the command line option into the produced object file. The option makes it trivial to trace back how a file was compiled and by which version of the gcc. It helps with debugging, reproducing bugs and repeating the build process.

This option is similar to -frecord-gcc-switches. However, they have three fundamental differences: Firstly, -frecord-gcc-switches saves the internal state after the argv is processed and passed by the driver. As opposed to that, --record-gcc-command-line saves the command-line as received by the driver. Secondly, -frecord-gcc-switches saves the switches as separate entries into a mergeable string section. Therefore, the entries belonging to different object files get mixed up after being linked. The new --record-gcc-command-line, on the other hand, creates one entry per invocation. By doing so, it makes it clear which options were used together in a single gcc invocation. Lastly, --record-gcc-command-line also adds the version of the gcc into this single entry to make it clear which version of gcc was called with any given command line. This is useful in cases where .comment section reports multiple versions.

While there are also similarities between the implementations of these two options, they are completely independent. These commands can be used separately or together without issues. I used the same section that -frecord-gcc-switches uses on purpose. I could not use the name -frecord-gcc-command-line for this option; because of a {f*} in the specs, which forwards all options starting with -f to cc1/cc1plus as is. This is not we want for this option. We would like to append it a filename as well to pass the argv of the driver to child processes.

This functionality operates as the following: It saves gcc's argv into a temporary file, and passes --record-gcc-command-line <tempfilename> to cc1 or cc1plus. The functionality of the backend is implemented via a hook. This patch includes an example implementation of the hook for elf targets: elf_record_gcc_command_line function. This function reads the given file and writes gcc's version and the command line into a mergeable string section, .GCC.command.line.

Here is an *example usage* of the option:
[egeyar@localhost save-commandline]$ gcc main.c --record-gcc-command-line
[egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out

String dump of section '.GCC.command.line':
  [     0]  10.0.0 20191025 (experimental) : gcc main.c --record-gcc-command-line


The following is a *second example* calling g++ with -save-temps, -frecord-gcc-switches, and repetition of options, where --save-temps saves the intermediate file, main.cmdline in this case. You can see that the options are recorded unprocessed:

[egeyar@localhost save-commandline]$ g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line
[egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out

String dump of section '.GCC.command.line':
  [     0]  10.0.0 20191025 (experimental) : g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line


Here is a *third example* calling g++ with both -frecord-gcc-switches and --record-gcc-command-line for comparison:
[egeyar@localhost save-commandline]$ g++ main.c --record-gcc-command-line -frecord-gcc-switches
[egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out

String dump of section '.GCC.command.line':
  [     0]  10.0.0 20191025 (experimental) : g++ main.c --record-gcc-command-line -frecord-gcc-switches
  [    5c]  -D_GNU_SOURCE
  [    6a]  main.c
  [    71]  -mtune=generic
  [    80]  -march=x86-64
  [    8e]  --record-gcc-command-line /tmp/ccgC4ZtS.cmdline


The first patch of this two-patch-series only extends the testsuite machinery, while the second patch implements this functionality and adds a test case for it. In addition to that new test case, I built binutils as my test case after passing this option to CFLAGS. The added .GCC.command.line section of ld listed many compile commands as expected. Tested on x86_64-pc-linux-gnu.

Please review the patches, let me know what you think and apply if appropriate.

Regards
Egeyar
 

Egeyar Bagcioglu (2):
  Introduce dg-require-target-object-format
  Introduce the gcc option --record-gcc-command-line

 gcc/common.opt                                     |  4 +++
 gcc/config/elfos.h                                 |  5 +++
 gcc/doc/tm.texi                                    | 22 ++++++++++++
 gcc/doc/tm.texi.in                                 |  4 +++
 gcc/gcc.c                                          | 41 ++++++++++++++++++++++
 gcc/gcc.h                                          |  1 +
 gcc/target.def                                     | 30 ++++++++++++++++
 gcc/target.h                                       |  3 ++
 .../c-c++-common/record-gcc-command-line.c         |  8 +++++
 gcc/testsuite/lib/target-supports-dg.exp           | 11 ++++++
 gcc/toplev.c                                       | 13 +++++++
 gcc/varasm.c                                       | 36 +++++++++++++++++++
 12 files changed, 178 insertions(+)
 create mode 100644 gcc/testsuite/c-c++-common/record-gcc-command-line.c

--
1.8.3.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Introduce dg-require-target-object-format

Egeyar Bagcioglu
gcc/testsuite/ChangeLog:
2019-11-06  Egeyar Bagcioglu  <[hidden email]>

        * lib/target-supports-dg.exp: Define dg-require-target-object-format.

---
 gcc/testsuite/lib/target-supports-dg.exp | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gcc/testsuite/lib/target-supports-dg.exp b/gcc/testsuite/lib/target-supports-dg.exp
index e1da57a..e923754 100644
--- a/gcc/testsuite/lib/target-supports-dg.exp
+++ b/gcc/testsuite/lib/target-supports-dg.exp
@@ -164,6 +164,17 @@ proc dg-require-dll { args } {
     set dg-do-what [list [lindex ${dg-do-what} 0] "N" "P"]
 }
 
+# If this target does not produce the given object format skip this test.
+
+proc dg-require-target-object-format { args } {
+    if { [gcc_target_object_format] == [lindex $args 1] } {
+ return
+    }
+
+    upvar dg-do-what dg-do-what
+    set dg-do-what [list [lindex ${dg-do-what} 0] "N" "P"]
+}
+
 # If this host does not support an ASCII locale, skip this test.
 
 proc dg-require-ascii-locale { args } {
--
1.8.3.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Introduce the gcc option --record-gcc-command-line

Egeyar Bagcioglu
In reply to this post by Egeyar Bagcioglu
gcc/ChangeLog:
2019-10-21  Egeyar Bagcioglu  <[hidden email]>

        * common.opt (--record-gcc-command-line): New option.
        * config/elfos.h (TARGET_ASM_RECORD_GCC_COMMAND_LINE): Define
        as elf_record_gcc_command_line.
        * doc/tm.texi: Regenerate.
        * doc/tm.texi.in (TARGET_ASM_RECORD_GCC_COMMAND_LINE): Introduce.
        (TARGET_ASM_RECORD_GCC_COMMAND_LINE_SECTION): Likewise.
        * gcc.c (_gcc_argc): New static variable.
        (_gcc_argv): Likewise.
        (record_gcc_command_line_spec_function): New function.
        (cc1_options): Add --record-gcc-command-line.
        (static_spec_functions): Add record_gcc_command_line_spec_function
        with pseudo name record-gcc-command-line.
        (driver::main): Call set_commandline.
        (driver::set_commandline): Declare.
        * gcc.h (driver::set_commandline): Declare.
        * target.def (record_gcc_command_line): A new hook.
        (record_gcc_command_line_section): A new hookpod.
        * target.h (elf_record_gcc_command_line): Declare.
        * toplev.c (init_asm_output): Check gcc_command_line_file and
        call record_gcc_command_line if necessary.
        * varasm.c: Include "version.h".
        (elf_record_gcc_command_line): Define.

gcc/testsuite/ChangeLog:
2019-10-30  Egeyar Bagcioglu  <[hidden email]>

        * c-c++-common/record-gcc-command-line.c: New test case.


---
 gcc/common.opt                                     |  4 +++
 gcc/config/elfos.h                                 |  5 +++
 gcc/doc/tm.texi                                    | 22 ++++++++++++
 gcc/doc/tm.texi.in                                 |  4 +++
 gcc/gcc.c                                          | 41 ++++++++++++++++++++++
 gcc/gcc.h                                          |  1 +
 gcc/target.def                                     | 30 ++++++++++++++++
 gcc/target.h                                       |  3 ++
 .../c-c++-common/record-gcc-command-line.c         |  8 +++++
 gcc/toplev.c                                       | 13 +++++++
 gcc/varasm.c                                       | 36 +++++++++++++++++++
 11 files changed, 167 insertions(+)
 create mode 100644 gcc/testsuite/c-c++-common/record-gcc-command-line.c

diff --git a/gcc/common.opt b/gcc/common.opt
index cc279f4..59d670f 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -394,6 +394,10 @@ Driver Alias(print-sysroot-headers-suffix)
 -profile
 Common Alias(p)
 
+-record-gcc-command-line
+Common Driver NoDriverArg Separate Var(gcc_command_line_file)
+Record the command line making this gcc call in the produced object file.
+
 -save-temps
 Driver Alias(save-temps)
 
diff --git a/gcc/config/elfos.h b/gcc/config/elfos.h
index e00d437..5caa9e0 100644
--- a/gcc/config/elfos.h
+++ b/gcc/config/elfos.h
@@ -451,6 +451,11 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #undef  TARGET_ASM_RECORD_GCC_SWITCHES
 #define TARGET_ASM_RECORD_GCC_SWITCHES elf_record_gcc_switches
 
+/* Allow the use of the --record-gcc-command-line switch via the
+   elf_record_gcc_command_line function defined in varasm.c.  */
+#undef  TARGET_ASM_RECORD_GCC_COMMAND_LINE
+#define TARGET_ASM_RECORD_GCC_COMMAND_LINE elf_record_gcc_command_line
+
 /* A C statement (sans semicolon) to output to the stdio stream STREAM
    any text necessary for declaring the name of an external symbol
    named NAME which is referenced in this compilation but not defined.
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 915e961..6da5e1b 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -8061,6 +8061,28 @@ ELF implementation of the @code{TARGET_ASM_RECORD_GCC_SWITCHES} target
 hook.
 @end deftypevr
 
+@deftypefn {Target Hook} int TARGET_ASM_RECORD_GCC_COMMAND_LINE ()
+Provides the target with the ability to record the command line that
+has been passed to the compiler driver. The @var{gcc_command_line_file}
+variable specifies the intermediate file that holds the command line.
+
+The return value must be zero.  Other return values may be supported
+in the future.
+
+By default this hook is set to NULL, but an example implementation,
+@var{elf_record_gcc_command_line}, is provided for ELF based targets.
+it records the command line as ASCII text inside a new, mergable string
+section in the assembler output file.  The name of the new section is
+provided by the @code{TARGET_ASM_RECORD_GCC_COMMAND_LINE_SECTION}
+target hook.
+@end deftypefn
+
+@deftypevr {Target Hook} {const char *} TARGET_ASM_RECORD_GCC_COMMAND_LINE_SECTION
+This is the name of the section that will be created by the example
+ELF implementation of the @code{TARGET_ASM_RECORD_GCC_COMMAND_LINE}
+target hook.
+@end deftypevr
+
 @need 2000
 @node Data Output
 @subsection Output of Data
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index ac0f049..73ca552 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -5192,6 +5192,10 @@ It must not be modified by command-line option processing.
 
 @hook TARGET_ASM_RECORD_GCC_SWITCHES_SECTION
 
+@hook TARGET_ASM_RECORD_GCC_COMMAND_LINE
+
+@hook TARGET_ASM_RECORD_GCC_COMMAND_LINE_SECTION
+
 @need 2000
 @node Data Output
 @subsection Output of Data
diff --git a/gcc/gcc.c b/gcc/gcc.c
index c45a1df..480263e 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -236,6 +236,11 @@ static int verbose_only_flag;
 
 static int print_subprocess_help;
 
+/* argc and argv used to call gcc. Necessary for
+   --record-gcc-command-line option.  */
+static int _gcc_argc;
+static const char **_gcc_argv;
+
 /* Linker suffix passed to -fuse-ld=... */
 static const char *use_ld;
 
@@ -409,6 +414,7 @@ static const char *replace_extension_spec_func (int, const char **);
 static const char *greater_than_spec_func (int, const char **);
 static const char *debug_level_greater_than_spec_func (int, const char **);
 static const char *find_fortran_preinclude_file (int, const char **);
+static const char *record_gcc_command_line_spec_function (int, const char **);
 static char *convert_white_space (char *);
 
 /* The Specs Language
@@ -1158,6 +1164,7 @@ static const char *cc1_options =
  %{!fsyntax-only:%{S:%W{o*}%{!o*:-o %b.s}}}\
  %{fsyntax-only:-o %j} %{-param*}\
  %{coverage:-fprofile-arcs -ftest-coverage}\
+ %{-record-gcc-command-line:--record-gcc-command-line %:record-gcc-command-line(%g.cmdline)}\
  %{fprofile-arcs|fprofile-generate*|coverage:\
    %{!fprofile-update=single:\
      %{pthread:-fprofile-update=prefer-atomic}}}";
@@ -1649,6 +1656,7 @@ static const struct spec_function static_spec_functions[] =
   { "gt", greater_than_spec_func },
   { "debug-level-gt", debug_level_greater_than_spec_func },
   { "fortran-preinclude-file", find_fortran_preinclude_file},
+  { "record-gcc-command-line", record_gcc_command_line_spec_function},
 #ifdef EXTRA_SPEC_FUNCTIONS
   EXTRA_SPEC_FUNCTIONS
 #endif
@@ -7359,6 +7367,7 @@ driver::main (int argc, char **argv)
   bool early_exit;
 
   set_progname (argv[0]);
+  set_commandline (argc, const_cast <const char **> (argv));
   expand_at_files (&argc, &argv);
   decode_argv (argc, const_cast <const char **> (argv));
   global_initializations ();
@@ -7402,6 +7411,15 @@ driver::set_progname (const char *argv0) const
   xmalloc_set_program_name (progname);
 }
 
+/* Keep command line for --record-gcc-command-line.  */
+
+void
+driver::set_commandline (int argc, const char **argv) const
+{
+  _gcc_argc = argc;
+  _gcc_argv = argv;
+}
+
 /* Expand any @ files within the command-line args,
    setting at_file_supplied if any were expanded.  */
 
@@ -10046,6 +10064,29 @@ find_fortran_preinclude_file (int argc, const char **argv)
   return result;
 }
 
+/* The option --record-gcc-command-line saves the command line options given to
+   gcc into the created object file. Those options are passed to the backends via
+   an intermediate file whose name is generated while the spec is evaluated. This
+   function takes that filename as an argument, writes to it, and returns it as a
+   single string.  */
+
+static const char *
+record_gcc_command_line_spec_function(int argc ATTRIBUTE_UNUSED, const char **argv)
+{
+  const char *filename = argv[0];
+  FILE *out = fopen (filename, "w");
+  if (out)
+    {
+      fputs (_gcc_argv[0], out);
+      for (int i = 1; i < _gcc_argc; i += 1)
+ {
+  fputc (' ', out);
+  fputs (_gcc_argv[i], out);
+ }
+      fclose (out);
+    }
+  return filename;
+}
 
 /* Insert backslash before spaces in ORIG (usually a file path), to
    avoid being broken by spec parser.
diff --git a/gcc/gcc.h b/gcc/gcc.h
index dc77dba..0ab0e93 100644
--- a/gcc/gcc.h
+++ b/gcc/gcc.h
@@ -37,6 +37,7 @@ class driver
 
  private:
   void set_progname (const char *argv0) const;
+  void set_commandline (int argc, const char **argv) const;
   void expand_at_files (int *argc, char ***argv) const;
   void decode_argv (int argc, const char **argv);
   void global_initializations ();
diff --git a/gcc/target.def b/gcc/target.def
index 1f011ed..62e1381 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -801,6 +801,36 @@ ELF implementation of the @code{TARGET_ASM_RECORD_GCC_SWITCHES} target\n\
 hook.",
  const char *, ".GCC.command.line")
 
+/* Output a record of the command line that have been passed to gcc.  */
+DEFHOOK
+(record_gcc_command_line,
+ "Provides the target with the ability to record the command line that\n\
+has been passed to the compiler driver. The @var{gcc_command_line_file}\n\
+variable specifies the intermediate file that holds the command line.\n\
+\n\
+The return value must be zero.  Other return values may be supported\n\
+in the future.\n\
+\n\
+By default this hook is set to NULL, but an example implementation,\n\
+@var{elf_record_gcc_command_line}, is provided for ELF based targets.\n\
+it records the command line as ASCII text inside a new, mergable string\n\
+section in the assembler output file.  The name of the new section is\n\
+provided by the @code{TARGET_ASM_RECORD_GCC_COMMAND_LINE_SECTION}\n\
+target hook.",
+ int, (),
+ NULL)
+
+/* The name of the section that the example ELF implementation of
+   record_gcc_switches will use to store the information.  Target
+   specific versions of record_gcc_switches may or may not use
+   this information.  */
+DEFHOOKPOD
+(record_gcc_command_line_section,
+ "This is the name of the section that will be created by the example\n\
+ELF implementation of the @code{TARGET_ASM_RECORD_GCC_COMMAND_LINE}\n\
+target hook.",
+ const char *, ".GCC.command.line")
+
 /* Output the definition of a section anchor.  */
 DEFHOOK
 (output_anchor,
diff --git a/gcc/target.h b/gcc/target.h
index 9f80658..4d180ec 100644
--- a/gcc/target.h
+++ b/gcc/target.h
@@ -101,6 +101,9 @@ typedef int (* print_switch_fn_type) (print_switch_type, const char *);
 /* An example implementation for ELF targets.  Defined in varasm.c  */
 extern int elf_record_gcc_switches (print_switch_type type, const char *);
 
+/* An example implementation for ELF targets.  Defined in varasm.c  */
+extern int elf_record_gcc_command_line ();
+
 /* Some places still assume that all pointer or address modes are the
    standard Pmode and ptr_mode.  These optimizations become invalid if
    the target actually supports multiple different modes.  For now,
diff --git a/gcc/testsuite/c-c++-common/record-gcc-command-line.c b/gcc/testsuite/c-c++-common/record-gcc-command-line.c
new file mode 100644
index 0000000..2fdce4f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/record-gcc-command-line.c
@@ -0,0 +1,8 @@
+/* Test the option "--record-gcc-command-line".
+   { dg-do compile }
+   { dg-require-target-object-format "elf" }
+   { dg-options "-O2 -g --record-gcc-command-line" } */
+
+int main () { return 0; }
+
+/* { dg-final { scan-assembler "-O2 -g --record-gcc-command-line"} }  */
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 1c7002f..3e1e1ef 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -866,6 +866,19 @@ init_asm_output (const char *name)
     {
       targetm.asm_out.file_start ();
 
+      if (gcc_command_line_file)
+ {
+  if (targetm.asm_out.record_gcc_command_line)
+    {
+      /* Record the command line.  */
+      targetm.asm_out.record_gcc_command_line ();
+    }
+  else
+    inform (UNKNOWN_LOCATION,
+    "--record-gcc-command-line is not supported by "
+    "the current target");
+ }
+
       if (flag_record_gcc_switches)
  {
   if (targetm.asm_out.record_gcc_switches)
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 57365ad..ff86ae8 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -43,6 +43,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "fold-const.h"
 #include "stor-layout.h"
 #include "varasm.h"
+#include "version.h"
 #include "flags.h"
 #include "stmt.h"
 #include "expr.h"
@@ -7819,6 +7820,41 @@ elf_record_gcc_switches (print_switch_type type, const char * name)
   return 0;
 }
 
+/* This function provides a possible implementation of the
+   TARGET_ASM_RECORD_GCC_COMMAND_LINE target hook for ELF targets.  When
+   triggered by --record-gcc-command-line it creates a new mergeable
+   string section in the assembler output file called
+   TARGET_ASM_RECORD_GCC_COMMAND_LINE_SECTION which contains the command line
+   in ASCII format.  */
+
+int
+elf_record_gcc_command_line ()
+{
+  char cmdline[256];
+  section * sec;
+  sec = get_section (targetm.asm_out.record_gcc_switches_section,
+     SECTION_DEBUG | SECTION_MERGE
+     | SECTION_STRINGS | (SECTION_ENTSIZE & 1),
+     NULL);
+  switch_to_section (sec);
+
+  ASM_OUTPUT_ASCII(asm_out_file, version_string, strlen(version_string));
+
+  FILE *out_stream = fopen (gcc_command_line_file, "r");
+  if (out_stream)
+    {
+      ASM_OUTPUT_ASCII(asm_out_file, " : ", 3);
+      ssize_t cmdline_length;
+      while ((cmdline_length = fread(cmdline, 1, 256, out_stream)))
+ ASM_OUTPUT_ASCII(asm_out_file, cmdline, cmdline_length);
+    }
+  cmdline[0] = 0;
+  ASM_OUTPUT_ASCII(asm_out_file, cmdline, 1);
+
+  /* The return value is currently ignored by the caller, but must be 0.  */
+  return 0;
+}
+
 /* Emit text to declare externally defined symbols. It is needed to
    properly support non-default visibility.  */
 void
--
1.8.3.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Introduce dg-require-target-object-format

Segher Boessenkool
In reply to this post by Egeyar Bagcioglu
Hi!

On Wed, Nov 06, 2019 at 06:21:33PM +0100, Egeyar Bagcioglu wrote:
> gcc/testsuite/ChangeLog:
> 2019-11-06  Egeyar Bagcioglu  <[hidden email]>
>
>         * lib/target-supports-dg.exp: Define dg-require-target-object-format.

* lib/target-supports-dg.exp (dg-require-target-object-format): New.

> +proc dg-require-target-object-format { args } {
> +    if { [gcc_target_object_format] == [lindex $args 1] } {
> + return
> +    }

"==" for strings is dangerous.  Use "eq" or "string equal"?


Segher
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Introduce the gcc option --record-gcc-command-line

Segher Boessenkool
In reply to this post by Egeyar Bagcioglu
Hi!

On Wed, Nov 06, 2019 at 06:21:34PM +0100, Egeyar Bagcioglu wrote:

> +static const char *
> +record_gcc_command_line_spec_function(int argc ATTRIBUTE_UNUSED, const char **argv)
> +{
> +  const char *filename = argv[0];
> +  FILE *out = fopen (filename, "w");
> +  if (out)
> +    {
> +      fputs (_gcc_argv[0], out);
> +      for (int i = 1; i < _gcc_argc; i += 1)
> + {
> +  fputc (' ', out);
> +  fputs (_gcc_argv[i], out);
> + }
> +      fclose (out);
> +    }
> +  return filename;
> +}

Pet peeve: just use fprintf?

If there is an error, should this return 0 or indicate error some way?
Making the error silent ("if (out)") seems weird, otherwise -- if it is
on purpose, a comment might help.

> +int
> +elf_record_gcc_command_line ()
> +{
> +  char cmdline[256];
> +  section * sec;

section *sec;

> +  sec = get_section (targetm.asm_out.record_gcc_switches_section,
> +     SECTION_DEBUG | SECTION_MERGE
> +     | SECTION_STRINGS | (SECTION_ENTSIZE & 1),
> +     NULL);
> +  switch_to_section (sec);
> +
> +  ASM_OUTPUT_ASCII(asm_out_file, version_string, strlen(version_string));
> +
> +  FILE *out_stream = fopen (gcc_command_line_file, "r");
> +  if (out_stream)
> +    {
> +      ASM_OUTPUT_ASCII(asm_out_file, " : ", 3);
> +      ssize_t cmdline_length;
> +      while ((cmdline_length = fread(cmdline, 1, 256, out_stream)))

fread (
(and many more like it).

> + ASM_OUTPUT_ASCII(asm_out_file, cmdline, cmdline_length);
> +    }
> +  cmdline[0] = 0;
> +  ASM_OUTPUT_ASCII(asm_out_file, cmdline, 1);
> +
> +  /* The return value is currently ignored by the caller, but must be 0.  */
> +  return 0;
> +}

A temporary file like this isn't so great.  Opening a file as "r" but then
accessing it with "fread" is peculiar, too.

HTH,


Segher
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Martin Liška-2
In reply to this post by Egeyar Bagcioglu
On 11/6/19 6:21 PM, Egeyar Bagcioglu wrote:
> Hello,

Hello.

>
> I would like to propose the following patches which introduce a compile option --record-gcc-command-line. When passed to gcc, it saves the command line option into the produced object file. The option makes it trivial to trace back how a file was compiled and by which version of the gcc. It helps with debugging, reproducing bugs and repeating the build process.

I like your motivation, we as SUSE would like to have a similar functionality. But the current approach has some limitations that make it not usable (will explain later).

>
> This option is similar to -frecord-gcc-switches. However, they have three fundamental differences: Firstly, -frecord-gcc-switches saves the internal state after the argv is processed and passed by the driver. As opposed to that, --record-gcc-command-line saves the

I would not name it as a fundamental changes, it's doing very similar to what -frecord-gcc-switches does. Moreover, we also have one another option -grecord-gcc-switches
that saves command line into DWARF. Plus there's a Red Hat plugin called Annobin:
https://www.youtube.com/watch?v=uzffr1M-w5M
https://developers.redhat.com/blog/2018/02/20/annobin-storing-information-binaries/

Main limitation of current approach (and probably the suggested patch) are:
a) it does not print per function options, which can be modified with __attribute__ (or pragma):

$ cat demo.c
int foo()
{
   return 1;
}

#pragma GCC optimize ("-O3")

int bar()
{
   return 0;
}

int main()
{
   return 0;
}

b) we as SUSE are switching to LTO (-flto); doing that each LTO LTRANS will become one compilation unit and
one will see a misleading command line invocation:

$ gcc -flto -O2 demo2.c -c
$ gcc -flto -O3 demo.c -c
$ gcc demo.o demo2.o -o a.out -frecord-gcc-switches
...
        .file "<artificial>"
        .section .GCC.command.line,"MS",@progbits,1
        .ascii "-mtune=generic"
        .zero 1
        .ascii "-march=x86-64"
        .zero 1
        .ascii "-auxbase-strip a.out.ltrans0.ltrans.o"
        .zero 1
        .ascii "-O3"
        .zero 1
        .ascii "-fno-openmp"
        .zero 1
        .ascii "-fno-openacc"
        .zero 1
        .ascii "-fno-pie"
        .zero 1
        .ascii "-frecord-gcc-switches"
        .zero 1
        .ascii "-fltrans"
        .zero 1
        .ascii "a.out.ltrans0.o"
        .zero 1
        .text
        .type foo, @function

c) Current option recording is missing macros, which can influence compilation significantly:
-D_FORTIFY_SOURCE=2

Martin

command-line as received by the driver. Secondly, -frecord-gcc-switches saves the switches as separate entries into a mergeable string section. Therefore, the entries belonging to different object files get mixed up after being linked. The new --record-gcc-command-line, on the other hand, creates one entry per invocation. By doing so, it makes it clear which options were used together in a single gcc invocation. Lastly, --record-gcc-command-line also adds the version of the gcc into this single entry to make it clear which version of gcc was called with any given command line. This is useful in cases where .comment section reports multiple versions.

>
> While there are also similarities between the implementations of these two options, they are completely independent. These commands can be used separately or together without issues. I used the same section that -frecord-gcc-switches uses on purpose. I could not use the name -frecord-gcc-command-line for this option; because of a {f*} in the specs, which forwards all options starting with -f to cc1/cc1plus as is. This is not we want for this option. We would like to append it a filename as well to pass the argv of the driver to child processes.
>
> This functionality operates as the following: It saves gcc's argv into a temporary file, and passes --record-gcc-command-line <tempfilename> to cc1 or cc1plus. The functionality of the backend is implemented via a hook. This patch includes an example implementation of the hook for elf targets: elf_record_gcc_command_line function. This function reads the given file and writes gcc's version and the command line into a mergeable string section, .GCC.command.line.
>
> Here is an *example usage* of the option:
> [egeyar@localhost save-commandline]$ gcc main.c --record-gcc-command-line
> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>
> String dump of section '.GCC.command.line':
>    [     0]  10.0.0 20191025 (experimental) : gcc main.c --record-gcc-command-line
>
>
> The following is a *second example* calling g++ with -save-temps, -frecord-gcc-switches, and repetition of options, where --save-temps saves the intermediate file, main.cmdline in this case. You can see that the options are recorded unprocessed:
>
> [egeyar@localhost save-commandline]$ g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line
> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>
> String dump of section '.GCC.command.line':
>    [     0]  10.0.0 20191025 (experimental) : g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line
>
>
> Here is a *third example* calling g++ with both -frecord-gcc-switches and --record-gcc-command-line for comparison:
> [egeyar@localhost save-commandline]$ g++ main.c --record-gcc-command-line -frecord-gcc-switches
> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>
> String dump of section '.GCC.command.line':
>    [     0]  10.0.0 20191025 (experimental) : g++ main.c --record-gcc-command-line -frecord-gcc-switches
>    [    5c]  -D_GNU_SOURCE
>    [    6a]  main.c
>    [    71]  -mtune=generic
>    [    80]  -march=x86-64
>    [    8e]  --record-gcc-command-line /tmp/ccgC4ZtS.cmdline
>
>
> The first patch of this two-patch-series only extends the testsuite machinery, while the second patch implements this functionality and adds a test case for it. In addition to that new test case, I built binutils as my test case after passing this option to CFLAGS. The added .GCC.command.line section of ld listed many compile commands as expected. Tested on x86_64-pc-linux-gnu.
>
> Please review the patches, let me know what you think and apply if appropriate.
>
> Regards
> Egeyar
>  
>
> Egeyar Bagcioglu (2):
>    Introduce dg-require-target-object-format
>    Introduce the gcc option --record-gcc-command-line
>
>   gcc/common.opt                                     |  4 +++
>   gcc/config/elfos.h                                 |  5 +++
>   gcc/doc/tm.texi                                    | 22 ++++++++++++
>   gcc/doc/tm.texi.in                                 |  4 +++
>   gcc/gcc.c                                          | 41 ++++++++++++++++++++++
>   gcc/gcc.h                                          |  1 +
>   gcc/target.def                                     | 30 ++++++++++++++++
>   gcc/target.h                                       |  3 ++
>   .../c-c++-common/record-gcc-command-line.c         |  8 +++++
>   gcc/testsuite/lib/target-supports-dg.exp           | 11 ++++++
>   gcc/toplev.c                                       | 13 +++++++
>   gcc/varasm.c                                       | 36 +++++++++++++++++++
>   12 files changed, 178 insertions(+)
>   create mode 100644 gcc/testsuite/c-c++-common/record-gcc-command-line.c
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Martin Liška-2
+ adding the author of Annobin to the email thread

On 11/7/19 10:24 AM, Martin Liška wrote:
> a) it does not print per function options, which can be modified with __attribute__ (or pragma):

Compiler is aware of the information (and uses it in inlining (or ICF) for instance):

   cl_optimization *opt1 = opts_for_fn (decl);
   cl_optimization *opt2 = opts_for_fn (item->decl);

   if (opt1 != opt2 && !cl_optimization_option_eq (opt1, opt2))
     {
       if (dump_file && (dump_flags & TDF_DETAILS))
        {
          fprintf (dump_file, "optimization flags difference");
          cl_optimization_print_diff (dump_file, 2, opt1, opt2);
        }

       return return_false_with_msg ("optimization flags are different");
     }

Martin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Egeyar Bagcioglu
In reply to this post by Martin Liška-2

On 11/7/19 10:24 AM, Martin Liška wrote:
> On 11/6/19 6:21 PM, Egeyar Bagcioglu wrote:
>> Hello,
>
> Hello.

Thanks for your detailed reply Martin. You'll find my reply inline.
Since you added Nick Clifton to your following reply, I am adding him to
this email too. He is not only the author of annobin, he also submitted
the -frecord-gcc-switches to GCC. I agree that this discussion can
benefit from his input.

>
>> I would like to propose the following patches which introduce a
>> compile option --record-gcc-command-line. When passed to gcc, it
>> saves the command line option into the produced object file. The
>> option makes it trivial to trace back how a file was compiled and by
>> which version of the gcc. It helps with debugging, reproducing bugs
>> and repeating the build process.
>
> I like your motivation, we as SUSE would like to have a similar
> functionality. But the current approach has some limitations that make
> it not usable (will explain later).
I am glad you agree with the motivation. Let me answer below the other
concerns that you have.

>> This option is similar to -frecord-gcc-switches. However, they have
>> three fundamental differences: Firstly, -frecord-gcc-switches saves
>> the internal state after the argv is processed and passed by the
>> driver. As opposed to that, --record-gcc-command-line saves the
>
> I would not name it as a fundamental changes, it's doing very similar
> to what -frecord-gcc-switches does.

It is very similar; however, I still insist that what I outlined are
fundamental differences. As I mentioned in my previous email, I built
binutils as my test-case-project. I attach to this email the output of
"readelf -p .GCC.command.line ld/ld-new", so that you can see how well
the output is merged in general. Please take a look. It saves the
command line *as is* and as one entry per invocation.

For the record, this is just to test and showcase the functionality.
This patch in fact has nothing to do with binutils.

> Moreover, we also have one another option -grecord-gcc-switches
> that saves command line into DWARF.

As Nick also mentioned many times, -grecord-gcc-switches is in DWARF and
this causes a great disadvantage: it gets stripped out. I believe the
-grecord-gcc-switches is moot for the sake of this discussion. Because I
think the discussion surrounding the submission of -frecord-gcc-switches
makes it clear that the necessity to keep the compile options in the
object file is something that is already agreed on.

> Plus there's a Red Hat plugin called Annobin:
> https://www.youtube.com/watch?v=uzffr1M-w5M
> https://developers.redhat.com/blog/2018/02/20/annobin-storing-information-binaries/

I am aware of annobin, which is already released as a part of RHEL8. I
think it is much superior to what I am aiming here. The sole purpose of
this patch is to keep the command line options in the object file. I
believe this is a basic functionality that should be supported by the
GCC itself, without requiring a plugin. In other words, I think pushing
a different build of a GCC plugin for each GCC version we use on each
distro (i.e. versions-times-distros many plugin builds) is an overkill
for such a basic need.

Those who already use annobin for any of its many use cases, might of
course prefer it over this functionality. For the rest of the distros
and the GCC versions, I believe this patch is quite useful and
extendable for its quite basic purpose.

>
> Main limitation of current approach (and probably the suggested patch)
> are:
> a) it does not print per function options, which can be modified with
> __attribute__ (or pragma):
>
> $ cat demo.c
> int foo()
> {
>   return 1;
> }
>
> #pragma GCC optimize ("-O3")
>
> int bar()
> {
>   return 0;
> }
>
> int main()
> {
>   return 0;
> }
I understand the need here. However, the purpose of this patch is only
to save the command line options. Your example is a change in the source
file. Of course, the source file can change. Even the implementation of
the functions themselves might change. But I believe this is out of the
scope of this patch, which is the command line.

>
> b) we as SUSE are switching to LTO (-flto); doing that each LTO LTRANS
> will become one compilation unit and
> one will see a misleading command line invocation:
>
> $ gcc -flto -O2 demo2.c -c
> $ gcc -flto -O3 demo.c -c
> $ gcc demo.o demo2.o -o a.out -frecord-gcc-switches
> ...
>     .file    "<artificial>"
>     .section    .GCC.command.line,"MS",@progbits,1
>     .ascii    "-mtune=generic"
>     .zero    1
>     .ascii    "-march=x86-64"
>     .zero    1
>     .ascii    "-auxbase-strip a.out.ltrans0.ltrans.o"
>     .zero    1
>     .ascii    "-O3"
>     .zero    1
>     .ascii    "-fno-openmp"
>     .zero    1
>     .ascii    "-fno-openacc"
>     .zero    1
>     .ascii    "-fno-pie"
>     .zero    1
>     .ascii    "-frecord-gcc-switches"
>     .zero    1
>     .ascii    "-fltrans"
>     .zero    1
>     .ascii    "a.out.ltrans0.o"
>     .zero    1
>     .text
>     .type    foo, @function
This is a very interesting case indeed. Thanks for bringing it to my
attention. LTO seems to be discarding .GCC.command.line sections.

[egeyar@localhost lto]$ gcc -flto -O3 demo.c -c --record-gcc-command-line
[egeyar@localhost lto]$ readelf -p .GCC.command.line demo.o
String dump of section '.GCC.command.line':
   [     0]  10.0.0 20191025 (experimental) : gcc -flto -O3 demo.c -c
--record-gcc-command-line

[egeyar@localhost lto]$ gcc -flto -O2 demo2.c -c --record-gcc-command-line
[egeyar@localhost lto]$ readelf -p .GCC.command.line demo2.o
String dump of section '.GCC.command.line':
   [     0]  10.0.0 20191025 (experimental) : gcc -flto -O2 demo2.c -c
--record-gcc-command-line

[egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out
[egeyar@localhost lto]$ readelf -p .GCC.command.line a.out
readelf: a.out: Warning: Section '.GCC.command.line' was not dumped
because it does not exist!

[egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out
--record-gcc-command-line
[egeyar@localhost lto]$ readelf -p .GCC.command.line a.out
String dump of section '.GCC.command.line':
   [     0]  10.0.0 20191025 (experimental) : gcc @/tmp/ccSlWkxd

Running this last command with "-v" reveals that gcc is called twice
with temporary files. Only the .GCC.command.line of the second call
survives.

I believe the beauty of this implementation is that driver saves the
options *as it receives* in an intermediate file. It then passes this
-or not- to the child processes. I am not familiar with the internals of
LTO. However, thinking that those string sections can actually be merged
and output, this implementation can also be extended for lto1 to handle
the option correctly, and to suppress it for its calls to gcc.

>
> c) Current option recording is missing macros, which can influence
> compilation significantly:
> -D_FORTIFY_SOURCE=2

This is not the case for this patch:

[egeyar@localhost save-commandline]$ g++ main.c
--record-gcc-command-line -D_FORTIFY_SOURCE=2 -O3
[egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out

String dump of section '.GCC.command.line':
   [     0]  10.0.0 20191025 (experimental) : g++ main.c
--record-gcc-command-line -D_FORTIFY_SOURCE=2 -O3


The attached file showcases this as well. Please take a look, I think
you might find it useful.

Regards
Egeyar

> Martin
>
> command-line as received by the driver. Secondly,
> -frecord-gcc-switches saves the switches as separate entries into a
> mergeable string section. Therefore, the entries belonging to
> different object files get mixed up after being linked. The new
> --record-gcc-command-line, on the other hand, creates one entry per
> invocation. By doing so, it makes it clear which options were used
> together in a single gcc invocation. Lastly, --record-gcc-command-line
> also adds the version of the gcc into this single entry to make it
> clear which version of gcc was called with any given command line.
> This is useful in cases where .comment section reports multiple versions.
>>
>> While there are also similarities between the implementations of
>> these two options, they are completely independent. These commands
>> can be used separately or together without issues. I used the same
>> section that -frecord-gcc-switches uses on purpose. I could not use
>> the name -frecord-gcc-command-line for this option; because of a {f*}
>> in the specs, which forwards all options starting with -f to
>> cc1/cc1plus as is. This is not we want for this option. We would like
>> to append it a filename as well to pass the argv of the driver to
>> child processes.
>>
>> This functionality operates as the following: It saves gcc's argv
>> into a temporary file, and passes --record-gcc-command-line
>> <tempfilename> to cc1 or cc1plus. The functionality of the backend is
>> implemented via a hook. This patch includes an example implementation
>> of the hook for elf targets: elf_record_gcc_command_line function.
>> This function reads the given file and writes gcc's version and the
>> command line into a mergeable string section, .GCC.command.line.
>>
>> Here is an *example usage* of the option:
>> [egeyar@localhost save-commandline]$ gcc main.c
>> --record-gcc-command-line
>> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>>
>> String dump of section '.GCC.command.line':
>>    [     0]  10.0.0 20191025 (experimental) : gcc main.c
>> --record-gcc-command-line
>>
>>
>> The following is a *second example* calling g++ with -save-temps,
>> -frecord-gcc-switches, and repetition of options, where --save-temps
>> saves the intermediate file, main.cmdline in this case. You can see
>> that the options are recorded unprocessed:
>>
>> [egeyar@localhost save-commandline]$ g++ main.c -save-temps
>> --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line
>> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>>
>> String dump of section '.GCC.command.line':
>>    [     0]  10.0.0 20191025 (experimental) : g++ main.c -save-temps
>> --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line
>>
>>
>> Here is a *third example* calling g++ with both -frecord-gcc-switches
>> and --record-gcc-command-line for comparison:
>> [egeyar@localhost save-commandline]$ g++ main.c
>> --record-gcc-command-line -frecord-gcc-switches
>> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>>
>> String dump of section '.GCC.command.line':
>>    [     0]  10.0.0 20191025 (experimental) : g++ main.c
>> --record-gcc-command-line -frecord-gcc-switches
>>    [    5c]  -D_GNU_SOURCE
>>    [    6a]  main.c
>>    [    71]  -mtune=generic
>>    [    80]  -march=x86-64
>>    [    8e]  --record-gcc-command-line /tmp/ccgC4ZtS.cmdline
>>
>>
>> The first patch of this two-patch-series only extends the testsuite
>> machinery, while the second patch implements this functionality and
>> adds a test case for it. In addition to that new test case, I built
>> binutils as my test case after passing this option to CFLAGS. The
>> added .GCC.command.line section of ld listed many compile commands as
>> expected. Tested on x86_64-pc-linux-gnu.
>>
>> Please review the patches, let me know what you think and apply if
>> appropriate.
>>
>> Regards
>> Egeyar
>>
>> Egeyar Bagcioglu (2):
>>    Introduce dg-require-target-object-format
>>    Introduce the gcc option --record-gcc-command-line
>>
>>   gcc/common.opt                                     |  4 +++
>>   gcc/config/elfos.h                                 |  5 +++
>>   gcc/doc/tm.texi                                    | 22 ++++++++++++
>>   gcc/doc/tm.texi.in                                 |  4 +++
>>   gcc/gcc.c                                          | 41
>> ++++++++++++++++++++++
>>   gcc/gcc.h                                          |  1 +
>>   gcc/target.def                                     | 30
>> ++++++++++++++++
>>   gcc/target.h                                       |  3 ++
>>   .../c-c++-common/record-gcc-command-line.c         |  8 +++++
>>   gcc/testsuite/lib/target-supports-dg.exp           | 11 ++++++
>>   gcc/toplev.c                                       | 13 +++++++
>>   gcc/varasm.c                                       | 36
>> +++++++++++++++++++
>>   12 files changed, 178 insertions(+)
>>   create mode 100644
>> gcc/testsuite/c-c++-common/record-gcc-command-line.c
>>
>


ld-.GCC.command.line.out (117K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Nick Clifton
Hi Egeyar,

Thanks for including me in this discussion.

>>> This option is similar to -frecord-gcc-switches.

For the record I will also note that there is -fverbose-asm which
does almost the same thing, but only records the options as comments
in the assembler.  They are never converted into data in the actual
object files.

It is also worth noting that if your goal is to record how a binary
was produced, possibly with an eye to reproducibility, then you may
also need to record some environment variables too.

One thing I found with annobin is that capturing preprocessor options
(eg -D_FORTIFY_SOURCE) can be quite hard from inside gcc, since often
they have already been processed and discarded.  I do not know if this
affects your actual patch though.

Speaking of annobin, I will bang the gcc plugin gong again here and say
that if your patch is rejected then you might want to consider turning
it into a plugin instead.  In that way you will not need approval from
the gcc maintainers.  But of course you will have to maintain and
publicise the plugin yourself.

One other thought occurs to me, which is that if the patch is acceptable,
or at least the idea of it, then maybe it would be better to amalgamate
all of the current command line recording options into a single version.
Eg:

  --frecord-options=[dwarf,assembler,object]

where:

  --frecord-options=dwarf      is a synonym for -grecord-switches
  --frecord-options=assembler  is a synonym for -fverbose-asm
  --frecord-options=object     is a synonym for your option

The user could supply one or more of the selectors to have the recording
happen in multiple places.

Just an idea.

Cheers
  Nick

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Egeyar Bagcioglu


On 11/7/19 4:13 PM, Nick Clifton wrote:
> Hi Egeyar,
>
> Thanks for including me in this discussion.
>
>>>> This option is similar to -frecord-gcc-switches.
> For the record I will also note that there is -fverbose-asm which
> does almost the same thing, but only records the options as comments
> in the assembler.  They are never converted into data in the actual
> object files.

Right.

> It is also worth noting that if your goal is to record how a binary
> was produced, possibly with an eye to reproducibility, then you may
> also need to record some environment variables too.

That is an important point and in fact, such a need might arise. Even in
that case, I would like to keep the options of GCC as modular as
possible so that we can pick and drop as the specific use cases require.
This one is the one that saves the command line for now.

If we implement the aliases you suggested at the end, we can even create
aliases that combine them for the user.

> One thing I found with annobin is that capturing preprocessor options
> (eg -D_FORTIFY_SOURCE) can be quite hard from inside gcc, since often
> they have already been processed and discarded.  I do not know if this
> affects your actual patch though.

Yes, this was one of Martin's points as well. It is not the case for
this patch, though. I have noticed that the current options aim to
capture more than the command line, dive into GCC, and therefore miss or
discard some options given by the user. This patch only stores *argv* as
the driver receives and writes it to the object file blindly. Therefore,
capturing options such as -D_FORTIFY_SOURCE is no special case. Really,
this patch only answers the simple question of "How did you call GCC?".

> Speaking of annobin, I will bang the gcc plugin gong again here and say
> that if your patch is rejected then you might want to consider turning
> it into a plugin instead.  In that way you will not need approval from
> the gcc maintainers.  But of course you will have to maintain and
> publicise the plugin yourself.

Thanks for the suggestion. That will always be in my mind for more
ambitious cases. In the case of this specific 160-line patch though, I
believe it wouldn't bother us to maintain one more small patch in the
GCC packages we distribute. It can be "only at Oracle!". However, for me
this is really a basic functionality. Intel's icc has the most similar
-sox option too. Thinking back on how many times I said "now, how did we
compile this?" in the past, I would like this to be available for all
GCC users too, in the spirit of sharing.

> One other thought occurs to me, which is that if the patch is acceptable,
> or at least the idea of it, then maybe it would be better to amalgamate
> all of the current command line recording options into a single version.
> Eg:
>
>    --frecord-options=[dwarf,assembler,object]
>
> where:
>
>    --frecord-options=dwarf      is a synonym for -grecord-switches
>    --frecord-options=assembler  is a synonym for -fverbose-asm
>    --frecord-options=object     is a synonym for your option
>
> The user could supply one or more of the selectors to have the recording
> happen in multiple places.
>
> Just an idea.

This is a very good idea for the user experience! I already pass an
argument to cc1; however, we can always simplify the arguments of the
driver so that these similar functionalities can be called via one
common name plus an option. I really like the idea.

> Cheers
>    Nick
>

Thanks Nick!

Regards
Egeyar
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Introduce dg-require-target-object-format

Egeyar Bagcioglu
In reply to this post by Segher Boessenkool
Hi Segher!


On 11/7/19 8:47 AM, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Nov 06, 2019 at 06:21:33PM +0100, Egeyar Bagcioglu wrote:
>> gcc/testsuite/ChangeLog:
>> 2019-11-06  Egeyar Bagcioglu  <[hidden email]>
>>
>>          * lib/target-supports-dg.exp: Define dg-require-target-object-format.
> * lib/target-supports-dg.exp (dg-require-target-object-format): New.

Right, thanks for the correction!

>> +proc dg-require-target-object-format { args } {
>> +    if { [gcc_target_object_format] == [lindex $args 1] } {
>> + return
>> +    }
> "==" for strings is dangerous.  Use "eq" or "string equal"?

I see many "string match"es. I will make the change.

Just as a note, though: Why is it dangerous? In C, for example, this
would be a pointer comparison and consistently fail. In many other
languages, it is indeed a string comparison and works well.

I am asking also because I see "==" for variable vs literal strings in
gcc/testsuite/lib. As opposed to C-like languages that consistently fail
them, these seem to work. If you still think this is dangerous, I'd love
to know why. Also, if so, someone might want to check the library.

Thanks for the review!

Egeyar

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Introduce dg-require-target-object-format

Jose E. Marchesi
   
    On 11/7/19 8:47 AM, Segher Boessenkool wrote:
    > Hi!
    >
    > On Wed, Nov 06, 2019 at 06:21:33PM +0100, Egeyar Bagcioglu wrote:
    >> gcc/testsuite/ChangeLog:
    >> 2019-11-06  Egeyar Bagcioglu  <[hidden email]>
    >>
    >>          * lib/target-supports-dg.exp: Define dg-require-target-object-format.
    > * lib/target-supports-dg.exp (dg-require-target-object-format): New.
   
    Right, thanks for the correction!
   
    >> +proc dg-require-target-object-format { args } {
    >> +    if { [gcc_target_object_format] == [lindex $args 1] } {
    >> + return
    >> +    }
    > "==" for strings is dangerous.  Use "eq" or "string equal"?
   
    I see many "string match"es. I will make the change.
   
    Just as a note, though: Why is it dangerous? In C, for example, this
    would be a pointer comparison and consistently fail. In many other
    languages, it is indeed a string comparison and works well.
   
    I am asking also because I see "==" for variable vs literal strings in
    gcc/testsuite/lib. As opposed to C-like languages that consistently
    fail them, these seem to work. If you still think this is dangerous,
    I'd love to know why. Also, if so, someone might want to check the
    library.

Because if the string happens to look like a number Tcl will perform
arithmetic comparison instead of lexicographic comparison, i.e. "01" ==
"1" evaluates to true :)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Introduce dg-require-target-object-format

Egeyar Bagcioglu


On 11/7/19 6:17 PM, [hidden email] wrote:

>      
>      On 11/7/19 8:47 AM, Segher Boessenkool wrote:
>      > Hi!
>      >
>      > On Wed, Nov 06, 2019 at 06:21:33PM +0100, Egeyar Bagcioglu wrote:
>      >> gcc/testsuite/ChangeLog:
>      >> 2019-11-06  Egeyar Bagcioglu  <[hidden email]>
>      >>
>      >>          * lib/target-supports-dg.exp: Define dg-require-target-object-format.
>      > * lib/target-supports-dg.exp (dg-require-target-object-format): New.
>      
>      Right, thanks for the correction!
>      
>      >> +proc dg-require-target-object-format { args } {
>      >> +    if { [gcc_target_object_format] == [lindex $args 1] } {
>      >> + return
>      >> +    }
>      > "==" for strings is dangerous.  Use "eq" or "string equal"?
>      
>      I see many "string match"es. I will make the change.
>      
>      Just as a note, though: Why is it dangerous? In C, for example, this
>      would be a pointer comparison and consistently fail. In many other
>      languages, it is indeed a string comparison and works well.
>      
>      I am asking also because I see "==" for variable vs literal strings in
>      gcc/testsuite/lib. As opposed to C-like languages that consistently
>      fail them, these seem to work. If you still think this is dangerous,
>      I'd love to know why. Also, if so, someone might want to check the
>      library.
>
> Because if the string happens to look like a number Tcl will perform
> arithmetic comparison instead of lexicographic comparison, i.e. "01" ==
> "1" evaluates to true :)

Oh lovely indeed! I am glad I asked.
Thanks Jose!
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Introduce the gcc option --record-gcc-command-line

Egeyar Bagcioglu
In reply to this post by Segher Boessenkool
Hello again Segher!

On 11/7/19 9:03 AM, Segher Boessenkool wrote:

> Hi!
>
> On Wed, Nov 06, 2019 at 06:21:34PM +0100, Egeyar Bagcioglu wrote:
>> +static const char *
>> +record_gcc_command_line_spec_function(int argc ATTRIBUTE_UNUSED, const char **argv)
>> +{
>> +  const char *filename = argv[0];
>> +  FILE *out = fopen (filename, "w");
>> +  if (out)
>> +    {
>> +      fputs (_gcc_argv[0], out);
>> +      for (int i = 1; i < _gcc_argc; i += 1)
>> + {
>> +  fputc (' ', out);
>> +  fputs (_gcc_argv[i], out);
>> + }
>> +      fclose (out);
>> +    }
>> +  return filename;
>> +}
> Pet peeve: just use fprintf?

okay.

> If there is an error, should this return 0 or indicate error some way?
> Making the error silent ("if (out)") seems weird, otherwise -- if it is
> on purpose, a comment might help.

It was on purpose so that this does not interfere with the builds.
However, re-watching today Nick's talk at Cauldron where he mentions
it's good to fail even in such cases, I am rethinking if we would like
to error out here. If anyone has any preference on this, I'd like to hear.

>> +int
>> +elf_record_gcc_command_line ()
>> +{
>> +  char cmdline[256];
>> +  section * sec;
> section *sec;

right.

>> +  sec = get_section (targetm.asm_out.record_gcc_switches_section,
>> +     SECTION_DEBUG | SECTION_MERGE
>> +     | SECTION_STRINGS | (SECTION_ENTSIZE & 1),
>> +     NULL);
>> +  switch_to_section (sec);
>> +
>> +  ASM_OUTPUT_ASCII(asm_out_file, version_string, strlen(version_string));
>> +
>> +  FILE *out_stream = fopen (gcc_command_line_file, "r");
>> +  if (out_stream)
>> +    {
>> +      ASM_OUTPUT_ASCII(asm_out_file, " : ", 3);
>> +      ssize_t cmdline_length;
>> +      while ((cmdline_length = fread(cmdline, 1, 256, out_stream)))
> fread (
> (and many more like it).

okay, I will fix them. Thanks for catching.

>> + ASM_OUTPUT_ASCII(asm_out_file, cmdline, cmdline_length);
>> +    }
>> +  cmdline[0] = 0;
>> +  ASM_OUTPUT_ASCII(asm_out_file, cmdline, 1);
>> +
>> +  /* The return value is currently ignored by the caller, but must be 0.  */
>> +  return 0;
>> +}
> A temporary file like this isn't so great.

GCC operates with temporary files, doesn't it? What is the concern that
is specific to this one? That is the most reasonable way I found to pass
the argv of gcc to child processes for saving. Other ways of passing it
that I could think of, or the idea of saving it in the driver were
actually very bad ideas.

> Opening a file as "r" but then
> accessing it with "fread" is peculiar, too.

I am not sure what you mean here. Is it that you prefer "wb" and "rb"
instead of "w" and "r"? I thought it was enough to use a consistent pair.

> HTH,

It does! Thanks a lot for the review!

Regards
Egeyar
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Introduce the gcc option --record-gcc-command-line

Segher Boessenkool
Hi!

On Thu, Nov 07, 2019 at 06:44:17PM +0100, Egeyar Bagcioglu wrote:

> On 11/7/19 9:03 AM, Segher Boessenkool wrote:
> >>+ ASM_OUTPUT_ASCII(asm_out_file, cmdline, cmdline_length);
> >>+    }
> >>+  cmdline[0] = 0;
> >>+  ASM_OUTPUT_ASCII(asm_out_file, cmdline, 1);
> >>+
> >>+  /* The return value is currently ignored by the caller, but must be 0.
> >>*/
> >>+  return 0;
> >>+}
> >A temporary file like this isn't so great.
>
> GCC operates with temporary files, doesn't it? What is the concern that
> is specific to this one? That is the most reasonable way I found to pass
> the argv of gcc to child processes for saving. Other ways of passing it
> that I could think of, or the idea of saving it in the driver were
> actually very bad ideas.

Oh, this is for passing something to another process?  I guess I didn't
read it closely enough, sorry, never mind.

> >Opening a file as "r" but then
> >accessing it with "fread" is peculiar, too.
>
> I am not sure what you mean here. Is it that you prefer "wb" and "rb"
> instead of "w" and "r"? I thought it was enough to use a consistent pair.

I'd use fgets or similar, not fread.


Segher
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Introduce the gcc option --record-gcc-command-line

Egeyar Bagcioglu


On 11/7/19 7:57 PM, Segher Boessenkool wrote:

> Hi!
>
> On Thu, Nov 07, 2019 at 06:44:17PM +0100, Egeyar Bagcioglu wrote:
>> On 11/7/19 9:03 AM, Segher Boessenkool wrote:
>>>> + ASM_OUTPUT_ASCII(asm_out_file, cmdline, cmdline_length);
>>>> +    }
>>>> +  cmdline[0] = 0;
>>>> +  ASM_OUTPUT_ASCII(asm_out_file, cmdline, 1);
>>>> +
>>>> +  /* The return value is currently ignored by the caller, but must be 0.
>>>> */
>>>> +  return 0;
>>>> +}
>>> A temporary file like this isn't so great.
>> GCC operates with temporary files, doesn't it? What is the concern that
>> is specific to this one? That is the most reasonable way I found to pass
>> the argv of gcc to child processes for saving. Other ways of passing it
>> that I could think of, or the idea of saving it in the driver were
>> actually very bad ideas.
> Oh, this is for passing something to another process?  I guess I didn't
> read it closely enough, sorry, never mind.
>
>>> Opening a file as "r" but then
>>> accessing it with "fread" is peculiar, too.
>> I am not sure what you mean here. Is it that you prefer "wb" and "rb"
>> instead of "w" and "r"? I thought it was enough to use a consistent pair.
> I'd use fgets or similar, not fread.

Two things made me prefer fread over fgets here:
1) Although I am reading a string, I do not need each read character to
be checked against newline. I just need to read till end-of-file.
2) fread returns the number of elements read which I later use. If I
used fgets, I'd need to call strlen or so afterwards to get the string size.

Let me know please if you disagree or if there are advantages /
disadvantages that I omit.

Regards
Egeyar
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Introduce the gcc option --record-gcc-command-line

Segher Boessenkool
On Fri, Nov 08, 2019 at 12:05:19AM +0100, Egeyar Bagcioglu wrote:

> On 11/7/19 7:57 PM, Segher Boessenkool wrote:
> >>>Opening a file as "r" but then
> >>>accessing it with "fread" is peculiar, too.
> >>I am not sure what you mean here. Is it that you prefer "wb" and "rb"
> >>instead of "w" and "r"? I thought it was enough to use a consistent pair.
> >I'd use fgets or similar, not fread.
>
> Two things made me prefer fread over fgets here:
> 1) Although I am reading a string, I do not need each read character to
> be checked against newline. I just need to read till end-of-file.
> 2) fread returns the number of elements read which I later use. If I
> used fgets, I'd need to call strlen or so afterwards to get the string size.
>
> Let me know please if you disagree or if there are advantages /
> disadvantages that I omit.

Somehow I thought fread works differently with text streams than it does.
Maybe because you don't often see fread on text streams :-)

Sorry for the noise,


Segher
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Martin Liška-2
In reply to this post by Egeyar Bagcioglu
On 11/7/19 3:50 PM, Egeyar Bagcioglu wrote:

>
> On 11/7/19 10:24 AM, Martin Liška wrote:
>> On 11/6/19 6:21 PM, Egeyar Bagcioglu wrote:
>>> Hello,
>>
>> Hello.
>
> Thanks for your detailed reply Martin. You'll find my reply inline. Since you added Nick Clifton to your following reply, I am adding him to this email too. He is not only the author of annobin, he also submitted the -frecord-gcc-switches to GCC. I agree that this discussion can benefit from his input.
>
>>
>>> I would like to propose the following patches which introduce a compile option --record-gcc-command-line. When passed to gcc, it saves the command line option into the produced object file. The option makes it trivial to trace back how a file was compiled and by which version of the gcc. It helps with debugging, reproducing bugs and repeating the build process.
>>
>> I like your motivation, we as SUSE would like to have a similar functionality. But the current approach has some limitations that make it not usable (will explain later).
>
> I am glad you agree with the motivation. Let me answer below the other concerns that you have.
>
>>> This option is similar to -frecord-gcc-switches. However, they have three fundamental differences: Firstly, -frecord-gcc-switches saves the internal state after the argv is processed and passed by the driver. As opposed to that, --record-gcc-command-line saves the
>>
>> I would not name it as a fundamental changes, it's doing very similar to what -frecord-gcc-switches does.
>
> It is very similar; however, I still insist that what I outlined are fundamental differences. As I mentioned in my previous email, I built binutils as my test-case-project. I attach to this email the output of "readelf -p .GCC.command.line ld/ld-new", so that you can see how well the output is merged in general. Please take a look. It saves the command line *as is* and as one entry per invocation.

Hello.

Ok, works for me and I'm glad you also wrote a counterpart for bintuils which can easily present the information to a user.

>
> For the record, this is just to test and showcase the functionality. This patch in fact has nothing to do with binutils.
>
>> Moreover, we also have one another option -grecord-gcc-switches
>> that saves command line into DWARF.
>
> As Nick also mentioned many times, -grecord-gcc-switches is in DWARF and this causes a great disadvantage: it gets stripped out.

Well, that's still something I disagree. I bet RedHat is similarly to openSUSE also building all packages with a debug info, which
is later stripped and put into a foo-devel package. That's why one can easily read the compile options from these sub-packages.
My motivation is to write a rpm linter check that will verify that all object files really used flags that we expect.


> I believe the -grecord-gcc-switches is moot for the sake of this discussion. Because I think the discussion surrounding the submission of -frecord-gcc-switches makes it clear that the necessity to keep the compile options in the object file is something that is already agreed on.
>
>> Plus there's a Red Hat plugin called Annobin:
>> https://www.youtube.com/watch?v=uzffr1M-w5M
>> https://developers.redhat.com/blog/2018/02/20/annobin-storing-information-binaries/
>
> I am aware of annobin, which is already released as a part of RHEL8. I think it is much superior to what I am aiming here. The sole purpose of this patch is to keep the command line options in the object file. I believe this is a basic functionality that should be supported by the GCC itself, without requiring a plugin.

I fully aggree with you.

> In other words, I think pushing a different build of a GCC plugin for each GCC version we use on each distro (i.e. versions-times-distros many plugin builds) is an overkill for such a basic need.

Yep.

>
> Those who already use annobin for any of its many use cases, might of course prefer it over this functionality. For the rest of the distros and the GCC versions, I believe this patch is quite useful and extendable for its quite basic purpose.
>
>>
>> Main limitation of current approach (and probably the suggested patch) are:
>> a) it does not print per function options, which can be modified with __attribute__ (or pragma):
>>
>> $ cat demo.c
>> int foo()
>> {
>>   return 1;
>> }
>>
>> #pragma GCC optimize ("-O3")
>>
>> int bar()
>> {
>>   return 0;
>> }
>>
>> int main()
>> {
>>   return 0;
>> }
>
> I understand the need here. However, the purpose of this patch is only to save the command line options. Your example is a change in the source file. Of course, the source file can change. Even the implementation of the functions themselves might change. But I believe this is out of the scope of this patch, which is the command line.

I can easily live with that.

>
>>
>> b) we as SUSE are switching to LTO (-flto); doing that each LTO LTRANS will become one compilation unit and
>> one will see a misleading command line invocation:
>>
>> $ gcc -flto -O2 demo2.c -c
>> $ gcc -flto -O3 demo.c -c
>> $ gcc demo.o demo2.o -o a.out -frecord-gcc-switches
>> ...
>>     .file    "<artificial>"
>>     .section    .GCC.command.line,"MS",@progbits,1
>>     .ascii    "-mtune=generic"
>>     .zero    1
>>     .ascii    "-march=x86-64"
>>     .zero    1
>>     .ascii    "-auxbase-strip a.out.ltrans0.ltrans.o"
>>     .zero    1
>>     .ascii    "-O3"
>>     .zero    1
>>     .ascii    "-fno-openmp"
>>     .zero    1
>>     .ascii    "-fno-openacc"
>>     .zero    1
>>     .ascii    "-fno-pie"
>>     .zero    1
>>     .ascii    "-frecord-gcc-switches"
>>     .zero    1
>>     .ascii    "-fltrans"
>>     .zero    1
>>     .ascii    "a.out.ltrans0.o"
>>     .zero    1
>>     .text
>>     .type    foo, @function
>
> This is a very interesting case indeed. Thanks for bringing it to my attention. LTO seems to be discarding .GCC.command.line sections.
>
> [egeyar@localhost lto]$ gcc -flto -O3 demo.c -c --record-gcc-command-line
> [egeyar@localhost lto]$ readelf -p .GCC.command.line demo.o
> String dump of section '.GCC.command.line':
>    [     0]  10.0.0 20191025 (experimental) : gcc -flto -O3 demo.c -c --record-gcc-command-line
>
> [egeyar@localhost lto]$ gcc -flto -O2 demo2.c -c --record-gcc-command-line
> [egeyar@localhost lto]$ readelf -p .GCC.command.line demo2.o
> String dump of section '.GCC.command.line':
>    [     0]  10.0.0 20191025 (experimental) : gcc -flto -O2 demo2.c -c --record-gcc-command-line
>
> [egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out
> [egeyar@localhost lto]$ readelf -p .GCC.command.line a.out
> readelf: a.out: Warning: Section '.GCC.command.line' was not dumped because it does not exist!
>
> [egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out --record-gcc-command-line
> [egeyar@localhost lto]$ readelf -p .GCC.command.line a.out
> String dump of section '.GCC.command.line':
>    [     0]  10.0.0 20191025 (experimental) : gcc @/tmp/ccSlWkxd
>
> Running this last command with "-v" reveals that gcc is called twice with temporary files. Only the .GCC.command.line of the second call survives.>
> I believe the beauty of this implementation is that driver saves the options *as it receives* in an intermediate file. It then passes this -or not- to the child processes. I am not familiar with the internals of LTO. However, thinking that those string sections can actually be merged and output, this implementation can also be extended for lto1 to handle the option correctly, and to suppress it for its calls to gcc.

As already mentioned, the following feature is crucial for me. LTO works in following steps:

1) you first generate LTO bytecode with -c -flto (so you have N input files for which you want
to report command lines)
2) Then you do so called WPA phase, you load all N files and stream bytecode to M files (usually to 128)
3) Then, these M files emit proper .s file and you eventually end up with an executable (or a shared library).

That said, you have to stream the parsed command line through LTO and eventually emit multiple records based
for each incoming N files.

>
>>
>> c) Current option recording is missing macros, which can influence compilation significantly:
>> -D_FORTIFY_SOURCE=2
>
> This is not the case for this patch:
>
> [egeyar@localhost save-commandline]$ g++ main.c --record-gcc-command-line -D_FORTIFY_SOURCE=2 -O3
> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>
> String dump of section '.GCC.command.line':
>    [     0]  10.0.0 20191025 (experimental) : g++ main.c --record-gcc-command-line -D_FORTIFY_SOURCE=2 -O3

That's fine, thanks.

May I please ask you to take a look at LTO deeper? The patch will really help us significantly.

Thanks,
Martin

>
>
> The attached file showcases this as well. Please take a look, I think you might find it useful.
>
> Regards
> Egeyar
>
>> Martin
>>
>> command-line as received by the driver. Secondly, -frecord-gcc-switches saves the switches as separate entries into a mergeable string section. Therefore, the entries belonging to different object files get mixed up after being linked. The new --record-gcc-command-line, on the other hand, creates one entry per invocation. By doing so, it makes it clear which options were used together in a single gcc invocation. Lastly, --record-gcc-command-line also adds the version of the gcc into this single entry to make it clear which version of gcc was called with any given command line. This is useful in cases where .comment section reports multiple versions.
>>>
>>> While there are also similarities between the implementations of these two options, they are completely independent. These commands can be used separately or together without issues. I used the same section that -frecord-gcc-switches uses on purpose. I could not use the name -frecord-gcc-command-line for this option; because of a {f*} in the specs, which forwards all options starting with -f to cc1/cc1plus as is. This is not we want for this option. We would like to append it a filename as well to pass the argv of the driver to child processes.
>>>
>>> This functionality operates as the following: It saves gcc's argv into a temporary file, and passes --record-gcc-command-line <tempfilename> to cc1 or cc1plus. The functionality of the backend is implemented via a hook. This patch includes an example implementation of the hook for elf targets: elf_record_gcc_command_line function. This function reads the given file and writes gcc's version and the command line into a mergeable string section, .GCC.command.line.
>>>
>>> Here is an *example usage* of the option:
>>> [egeyar@localhost save-commandline]$ gcc main.c --record-gcc-command-line
>>> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>>>
>>> String dump of section '.GCC.command.line':
>>>    [     0]  10.0.0 20191025 (experimental) : gcc main.c --record-gcc-command-line
>>>
>>>
>>> The following is a *second example* calling g++ with -save-temps, -frecord-gcc-switches, and repetition of options, where --save-temps saves the intermediate file, main.cmdline in this case. You can see that the options are recorded unprocessed:
>>>
>>> [egeyar@localhost save-commandline]$ g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line
>>> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>>>
>>> String dump of section '.GCC.command.line':
>>>    [     0]  10.0.0 20191025 (experimental) : g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 --record-gcc-command-line
>>>
>>>
>>> Here is a *third example* calling g++ with both -frecord-gcc-switches and --record-gcc-command-line for comparison:
>>> [egeyar@localhost save-commandline]$ g++ main.c --record-gcc-command-line -frecord-gcc-switches
>>> [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out
>>>
>>> String dump of section '.GCC.command.line':
>>>    [     0]  10.0.0 20191025 (experimental) : g++ main.c --record-gcc-command-line -frecord-gcc-switches
>>>    [    5c]  -D_GNU_SOURCE
>>>    [    6a]  main.c
>>>    [    71]  -mtune=generic
>>>    [    80]  -march=x86-64
>>>    [    8e]  --record-gcc-command-line /tmp/ccgC4ZtS.cmdline
>>>
>>>
>>> The first patch of this two-patch-series only extends the testsuite machinery, while the second patch implements this functionality and adds a test case for it. In addition to that new test case, I built binutils as my test case after passing this option to CFLAGS. The added .GCC.command.line section of ld listed many compile commands as expected. Tested on x86_64-pc-linux-gnu.
>>>
>>> Please review the patches, let me know what you think and apply if appropriate.
>>>
>>> Regards
>>> Egeyar
>>>
>>> Egeyar Bagcioglu (2):
>>>    Introduce dg-require-target-object-format
>>>    Introduce the gcc option --record-gcc-command-line
>>>
>>>   gcc/common.opt                                     |  4 +++
>>>   gcc/config/elfos.h                                 |  5 +++
>>>   gcc/doc/tm.texi                                    | 22 ++++++++++++
>>>   gcc/doc/tm.texi.in                                 |  4 +++
>>>   gcc/gcc.c                                          | 41 ++++++++++++++++++++++
>>>   gcc/gcc.h                                          |  1 +
>>>   gcc/target.def                                     | 30 ++++++++++++++++
>>>   gcc/target.h                                       |  3 ++
>>>   .../c-c++-common/record-gcc-command-line.c         |  8 +++++
>>>   gcc/testsuite/lib/target-supports-dg.exp           | 11 ++++++
>>>   gcc/toplev.c                                       | 13 +++++++
>>>   gcc/varasm.c                                       | 36 +++++++++++++++++++
>>>   12 files changed, 178 insertions(+)
>>>   create mode 100644 gcc/testsuite/c-c++-common/record-gcc-command-line.c
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Martin Liška-2
In reply to this post by Nick Clifton
On 11/7/19 4:13 PM, Nick Clifton wrote:

> Hi Egeyar,
>
> Thanks for including me in this discussion.
>
>>>> This option is similar to -frecord-gcc-switches.
>
> For the record I will also note that there is -fverbose-asm which
> does almost the same thing, but only records the options as comments
> in the assembler.  They are never converted into data in the actual
> object files.

Heh, we have even more options..

>
> It is also worth noting that if your goal is to record how a binary
> was produced, possibly with an eye to reproducibility, then you may
> also need to record some environment variables too.
>
> One thing I found with annobin is that capturing preprocessor options
> (eg -D_FORTIFY_SOURCE) can be quite hard from inside gcc, since often
> they have already been processed and discarded.  I do not know if this
> affects your actual patch though.
>
> Speaking of annobin, I will bang the gcc plugin gong again here and say
> that if your patch is rejected then you might want to consider turning
> it into a plugin instead.  In that way you will not need approval from
> the gcc maintainers.  But of course you will have to maintain and
> publicise the plugin yourself.
>
> One other thought occurs to me, which is that if the patch is acceptable,
> or at least the idea of it, then maybe it would be better to amalgamate
> all of the current command line recording options into a single version.
> Eg:
>
>    --frecord-options=[dwarf,assembler,object]
>
> where:
>
>    --frecord-options=dwarf      is a synonym for -grecord-switches
>    --frecord-options=assembler  is a synonym for -fverbose-asm
>    --frecord-options=object     is a synonym for your option

I really like the suggested option name unification.

Martin

>
> The user could supply one or more of the selectors to have the recording
> happen in multiple places.
>
> Just an idea.
>
> Cheers
>    Nick
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

Jeff Law
In reply to this post by Martin Liška-2
On 11/13/19 2:37 AM, Martin Liška wrote:

>>
>> As Nick also mentioned many times, -grecord-gcc-switches is in DWARF
>> and this causes a great disadvantage: it gets stripped out.
>
> Well, that's still something I disagree. I bet RedHat is similarly to
> openSUSE also building all packages with a debug info, which
> is later stripped and put into a foo-devel package. That's why one can
> easily read the compile options from these sub-packages.
> My motivation is to write a rpm linter check that will verify that all
> object files really used flags that we expect.
Right.  We inject -g into the default build flags.  We extract the
resultant debug info into a .debuginfo RPM.

The original motivation behind annobin was to verify how well the
injection mechanism worked.  We originally wanted to do something like
what Egeyar has done, but it's been proposed in the past and was highly
controversial.  Rather than fight that problem or have a Red Hat
specific patch, we built annobin/annocheck which (IMHO) handles this
kind of need quite well.


Jeff

12