| Message ID | 1583246660-6390-4-git-send-email-egeyar.bagcioglu@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce a new GCC option, --record-gcc-command-line | expand |
On Tue, Mar 3, 2020 at 5:41 PM Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com> wrote: > > This patch is for .GCC.command.line sections in LTO objects to be copied > into the final objects as in the following example: > > [egeyar@localhost lto]$ gcc -flto -O3 demo.c -c -g --record-gcc-command-line > [egeyar@localhost lto]$ gcc -flto -O2 demo2.c -c -g --record-gcc-command-line -DFORTIFY=2 > [egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out > [egeyar@localhost lto]$ readelf -p .GCC.command.line a.out > > String dump of section '.GCC.command.line': > [ 0] 10.0.1 20200227 (experimental) : gcc -flto -O3 demo.c -c -g --record-gcc-command-line > [ 56] 10.0.1 20200227 (experimental) : gcc -flto -O2 demo2.c -c -g --record-gcc-command-line -DFORTIFY=2 --record-gcc-command-line is not a FSF GCC option, there's -frecord-gcc-switches though which (also) populates .GCC.command.line OK. Thanks, Richard. > Regards > Egeyar > > libiberty: > 2020-02-27 Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com> > > * simple-object.c (handle_lto_debug_sections): Name > ".GCC.command.line" among debug sections to be copied over > from lto objects. > --- > libiberty/simple-object.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libiberty/simple-object.c b/libiberty/simple-object.c > index d9c648a..3b3ca9c 100644 > --- a/libiberty/simple-object.c > +++ b/libiberty/simple-object.c > @@ -298,6 +298,9 @@ handle_lto_debug_sections (const char *name, int rename) > COMDAT sections in objects produced by GCC. */ > else if (strcmp (name, ".comment") == 0) > return strcpy (newname, name); > + /* Copy over .GCC.command.line section under the same name if present. */ > + else if (strcmp (name, ".GCC.command.line") == 0) > + return strcpy (newname, name); > free (newname); > return NULL; > } > -- > 1.8.3.1 >
On 3/4/20 10:00 AM, Richard Biener wrote: > On Tue, Mar 3, 2020 at 5:41 PM Egeyar Bagcioglu > <egeyar.bagcioglu@oracle.com> wrote: >> This patch is for .GCC.command.line sections in LTO objects to be copied >> into the final objects as in the following example: >> >> [egeyar@localhost lto]$ gcc -flto -O3 demo.c -c -g --record-gcc-command-line >> [egeyar@localhost lto]$ gcc -flto -O2 demo2.c -c -g --record-gcc-command-line -DFORTIFY=2 >> [egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out >> [egeyar@localhost lto]$ readelf -p .GCC.command.line a.out >> >> String dump of section '.GCC.command.line': >> [ 0] 10.0.1 20200227 (experimental) : gcc -flto -O3 demo.c -c -g --record-gcc-command-line >> [ 56] 10.0.1 20200227 (experimental) : gcc -flto -O2 demo2.c -c -g --record-gcc-command-line -DFORTIFY=2 > --record-gcc-command-line is not a FSF GCC option, there's > -frecord-gcc-switches though which > (also) populates .GCC.command.line Right. This is also necessary for preserving the outcome of -frecord-gcc-switches option and that issue is reported to me by Martin Liska. > OK. Thanks Richard. I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. Regards Egeyar > > Thanks, > Richard. > >> Regards >> Egeyar >> >> libiberty: >> 2020-02-27 Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com> >> >> * simple-object.c (handle_lto_debug_sections): Name >> ".GCC.command.line" among debug sections to be copied over >> from lto objects. >> --- >> libiberty/simple-object.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/libiberty/simple-object.c b/libiberty/simple-object.c >> index d9c648a..3b3ca9c 100644 >> --- a/libiberty/simple-object.c >> +++ b/libiberty/simple-object.c >> @@ -298,6 +298,9 @@ handle_lto_debug_sections (const char *name, int rename) >> COMDAT sections in objects produced by GCC. */ >> else if (strcmp (name, ".comment") == 0) >> return strcpy (newname, name); >> + /* Copy over .GCC.command.line section under the same name if present. */ >> + else if (strcmp (name, ".GCC.command.line") == 0) >> + return strcpy (newname, name); >> free (newname); >> return NULL; >> } >> -- >> 1.8.3.1 >>
On Mär 04 2020, Richard Biener wrote: > --record-gcc-command-line is not a FSF GCC option, there's > -frecord-gcc-switches though which --record-gcc-command-line is translated to -frecord-gcc-switches by the driver. That happens for all double-dash options that match an -f option. Andreas.
On 3/4/20 4:34 PM, Andreas Schwab wrote: > On Mär 04 2020, Richard Biener wrote: > >> --record-gcc-command-line is not a FSF GCC option, there's >> -frecord-gcc-switches though which > --record-gcc-command-line is translated to -frecord-gcc-switches by the > driver. That happens for all double-dash options that match an -f > option. I think there is a misunderstanding here. I have just implemented --record-gcc-command-line. I have been testing it and no it does not act as -frecord-gcc-switches. Regards Egeyar > Andreas. >
On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: > Thanks Richard. > > I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. Can we please wait? I'm really convinced we do not want one another very similar functionality. I would definitely recommend to change the semantics of -frecord-gcc-switches to what the patchset does. That's why I added Nick as he's the author of the original implementation. Reasoning is provided in my previous email. Martin
On Wed, Mar 04, 2020 at 06:23:10PM +0100, Martin Liška wrote: > On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: > > Thanks Richard. > > > > I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. > > Can we please wait? I'm really convinced we do not want one another very similar > functionality. I would definitely recommend to change the semantics > of -frecord-gcc-switches to what the patchset does. > > That's why I added Nick as he's the author of the original implementation. > Reasoning is provided in my previous email. Also, what is the reason for storing the option in a file, rather than say putting them into an environment variable from which the backend can pick it up? I must say I don't really see advantages of this over -grecord-gcc-switches, recording all options looks very bloaty and will include mostly stuff you don't really care about (such as, e.g. the -I options without knowing what was the current directory when the source file has been compiled), on the other side will not record interesting options that -grecord-gcc-switches records (say, if some code is compiled with -march=native, this new option will record that, rather than what it really is), but I won't stand in a way unless such an option would be on by default. Jakub
On 3/4/20 6:33 PM, Jakub Jelinek wrote: > On Wed, Mar 04, 2020 at 06:23:10PM +0100, Martin Liška wrote: >> On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: >>> Thanks Richard. >>> >>> I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. >> >> Can we please wait? I'm really convinced we do not want one another very similar >> functionality. I would definitely recommend to change the semantics >> of -frecord-gcc-switches to what the patchset does. >> >> That's why I added Nick as he's the author of the original implementation. >> Reasoning is provided in my previous email. > > Also, what is the reason for storing the option in a file, rather than say > putting them into an environment variable from which the backend can pick it > up? That's smart suggestion! It would not be the first environment variable that we use, right? > I must say I don't really see advantages of this over > -grecord-gcc-switches, recording all options looks very bloaty and will > include mostly stuff you don't really care about (such as, e.g. the -I > options without knowing what was the current directory when the source file > has been compiled), on the other side will not record interesting options > that -grecord-gcc-switches records (say, if some code is compiled with > -march=native, this new option will record that, rather than what it really > is), but I won't stand in a way unless such an option would be on by > default. Yes, it's a minor disadvantage. On the other hand one can check the fortify macros. I don't care much about them too, but what's the biggest benefit to me is that each argument will not go into it's own mergeable section. Then you will not see something like: $ gcc -O0 foo.c -frecord-gcc-switches -c -g $ gcc -Ofast bar.c -frecord-gcc-switches -c -g0 $ gcc foo.o bar.o $ readelf -p .GCC.command.line a.out String dump of section '.GCC.command.line': [ 0] foo.c [ 6] -mtune=generic [ 15] -march=x86-64 [ 23] -g [ 26] -O0 [ 2a] -frecord-gcc-switches [ 40] bar.c [ 46] -g0 [ 4a] -Ofast The output is useless and can't disambiguate each compiler invocations. Martin > > Jakub >
On Wed, Mar 04, 2020 at 06:42:29PM +0100, Martin Liška wrote: > > I must say I don't really see advantages of this over > > -grecord-gcc-switches, recording all options looks very bloaty and will > > include mostly stuff you don't really care about (such as, e.g. the -I > > options without knowing what was the current directory when the source file > > has been compiled), on the other side will not record interesting options > > that -grecord-gcc-switches records (say, if some code is compiled with > > -march=native, this new option will record that, rather than what it really > > is), but I won't stand in a way unless such an option would be on by > > default. > > Yes, it's a minor disadvantage. On the other hand one can check the fortify > macros. I don't care much about them too, but what's the biggest benefit to me > is that each argument will not go into it's own mergeable section. Then > you will not see something like: Well, the fortify macro is questionable, because as a macro, it can be either specified on the command line, or e.g. defined in the source before including headers, so -g3 seems much better way to query it. > The output is useless and can't disambiguate each compiler > invocations. Sure, I'm not talking about -frecord-gcc-switches, that option is indeed not really useful. Jakub
On 3/4/20 6:23 PM, Martin Liška wrote: > On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: >> Thanks Richard. >> >> I do not have write-access to the GCC repo. I'd be glad if someone >> commits it for me. > > Can we please wait? I'm really convinced we do not want one another > very similar > functionality. I am sorry. I thought Richard's comment was only about the third patch, which is also necessary for the current -frecord-gcc-switches. Therefore, my reply was only for that patch as well. Regards Egeyar > I would definitely recommend to change the semantics > of -frecord-gcc-switches to what the patchset does. > > That's why I added Nick as he's the author of the original > implementation. > Reasoning is provided in my previous email. > > Martin
On 3/4/20 6:33 PM, Jakub Jelinek wrote: > On Wed, Mar 04, 2020 at 06:23:10PM +0100, Martin Liška wrote: >> On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: >>> Thanks Richard. >>> >>> I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. >> Can we please wait? I'm really convinced we do not want one another very similar >> functionality. I would definitely recommend to change the semantics >> of -frecord-gcc-switches to what the patchset does. >> >> That's why I added Nick as he's the author of the original implementation. >> Reasoning is provided in my previous email. > Also, what is the reason for storing the option in a file, rather than say > putting them into an environment variable from which the backend can pick it > up? I needed to pass that information from one process to the other and I picked a way to do so. Moreover, passing files is what gcc does; therefore, it didn't seem unnatural to do so. Furthermore, gcc specs already has its way to create temporary file names so I didn't have to worry about what level of thread safety is acceptable. If you tell me why an environment variable is better, we can discuss and I might implement that too. > I must say I don't really see advantages of this over > -grecord-gcc-switches, recording all options looks very bloaty and will > include mostly stuff you don't really care about (such as, e.g. the -I > options without knowing what was the current directory when the source file > has been compiled), on the other side will not record interesting options > that -grecord-gcc-switches records (say, if some code is compiled with > -march=native, this new option will record that, rather than what it really > is), 1) -grecord-gcc-switches is quite similar to -frecord-gcc-switches and my first and main arguement about the latter is valid for the former: You cannot easily construct the right command line to invoke the same compilation from the switches. Switches are useful to check that the compiler is doing the expected. The command line is useful when you want the compiler to do the same thing again. 2) The output is exactly what gcc takes as command line to make that compilation. It is bloaty, surely, when so is what's provided to gcc to make it compile as desired. On the other hand, -grecord-gcc-switches is just much bloatier since it does not work alone. It's embedded in dwarf. 3) Many systems automatically strip dwarf, together with this information. I do not think there is an easy way to separate this information and keep it. Since this comes up often, I'd like to emphasize that the internal switches of gcc are different than the user-given options of gcc. The purpose of this new option is really to catch the user given options of gcc and only them, so much so that I mark it SWITCH_IGNORE within the specs to avoid it propagated to any child processes of gcc. This option is simply to retrieve how the call was made to GCC. Currently, there are no other options giving us this information. > but I won't stand in a way unless such an option would be on by > default. I totally agree that this should not be on by default. Best regards Egeyar
On Wed, Mar 4, 2020 at 8:39 PM Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com> wrote: > > > > On 3/4/20 6:23 PM, Martin Liška wrote: > > On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: > >> Thanks Richard. > >> > >> I do not have write-access to the GCC repo. I'd be glad if someone > >> commits it for me. > > > > Can we please wait? I'm really convinced we do not want one another > > very similar > > functionality. > > I am sorry. I thought Richard's comment was only about the third patch, > which is also necessary for the current -frecord-gcc-switches. > Therefore, my reply was only for that patch as well. Yes, my comment / approval was only about preserving the early compile options for LTO. I'll commit that piece momentarily. Richard. > Regards > Egeyar > > > I would definitely recommend to change the semantics > > of -frecord-gcc-switches to what the patchset does. > > > > That's why I added Nick as he's the author of the original > > implementation. > > Reasoning is provided in my previous email. > > > > Martin >
Hi. I'm sending the updated patch based on Egeyar's work. It utilizes a new environmental variable and uses the currently existing -frecord-gcc-switches option. Thoughts? Martin
> I'm sending the updated patch based on Egeyar's work. > It utilizes a new environmental variable and uses the currently > existing -frecord-gcc-switches option. > > Thoughts? I am leaving it to the more experienced to comment on redefining the functionality of -frecord-gcc-switches. The code seems pretty neat to me. Thanks Martin! Best regards Egeyar
diff --git a/libiberty/simple-object.c b/libiberty/simple-object.c index d9c648a..3b3ca9c 100644 --- a/libiberty/simple-object.c +++ b/libiberty/simple-object.c @@ -298,6 +298,9 @@ handle_lto_debug_sections (const char *name, int rename) COMDAT sections in objects produced by GCC. */ else if (strcmp (name, ".comment") == 0) return strcpy (newname, name); + /* Copy over .GCC.command.line section under the same name if present. */ + else if (strcmp (name, ".GCC.command.line") == 0) + return strcpy (newname, name); free (newname); return NULL; }