Handle -g0 properly #371
Handle -g0 properly #371
Conversation
|
Oops, I was too fast merging this. And GitHub annoyingly doesn't allow me to reopen the pull request after reverting. 🙁 |
| @@ -2634,6 +2634,11 @@ cc_process_args(struct args *args, struct args **preprocessor_args, | |||
|
|
|||
| // Debugging is handled specially, so that we know if we can strip line | |||
| // number info. | |||
| if (str_eq(argv[i], "-g0")) { | |||
There was a problem hiding this comment.
This is not quite enough since -g3 -g0 still will enter the if statement below and for instance set conf->run_second_cpp = true.
And more importantly: since -g0 isn't added to stripped_args it won't be sent to the compiler so the compiler will then use -g3.
There was a problem hiding this comment.
Yes, there's also another problem with:
- conf->unify = true
- -g1 was parsed and conf->unify was set to false
- -g0 was parsed, but we can't restore conf->unify back to true, as there is no information of state of conf->unify before the -g1 caused it to be set to false
How do we solve this?
There was a problem hiding this comment.
Right. Thinking more about this, I now remember that we used to have smarter processing of -g* options (#92) but it was reverted (b8aeb98) due to being buggy (#149).
The best solution I see is to remember the last -g* option and only act (maybe disable unify, maybe set , etc.) after we have looped over all arguments. But it is quite complex to figure out how all options interact, so it's not clear to me how that should be implemented. For instance, -ggdb3 -g0 seems to be equivalent to -g3 -ggdb0, but -ggdb3 -g2 is not equivalent to -g3 -ggdb2...
So maybe we should keep things safe and not try to handle -g3 -g0 like I suggested. We could still handle the simple case in #368 like you did, but the argument then needs to be added to stripped_args.
Hmm... It does seem like -g*0 disables all debug info enabled by previous options, though, so maybe we could make use of that fact. I made an attempt in 749fa11 on a separate branch. What do you think about that approach?
There was a problem hiding this comment.
Looks good to me. Will you push it?
There was a problem hiding this comment.
Thanks for having a look. I'll push it after writing some tests.
| @@ -78,7 +78,7 @@ EOF | |||
| backdate "$BASEDIR3/header.h" "$BASEDIR3/test.c" | |||
| backdate "$BASEDIR4/header.h" "$BASEDIR4/test.c" "$BASEDIR4/header2.h" | |||
|
|
|||
| DEPFLAGS="-MD -MF test.d" | |||
| DEPFLAGS="-g1 -g0 -MD -MF test.d" | |||
There was a problem hiding this comment.
I would prefer a separate test case for this (in the base suite) to make it possible to understand to purpose of the flags.
|
@pavsa: I don't know if you got mail about my review comments since the PR has been closed already, so here's an extra ping that I left some feedback. |
This reverts commit afa4eea.
Solves #368
I have reused testing in depend mode as it has ideal setup this would fail.
Also removed the depend_different_headers.bash due to not being needed anymore.