Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553043002:
Don't disable debug symbols when building with sanitizers. (Closed)
|
Created:
4 years ago by mstensho (USE GERRIT) Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't disable debug symbols when building with sanitizers.
A -gline-tables-only specified after a -g will override the -g, so that
we end up with only line tables.
See http://clang.llvm.org/docs/UsersManual.html#controlling-size-of-debug-information
"If multiple flags are present, the last one is used."
Patch Set 1 #
Total comments: 3
Messages
Total messages: 23 (6 generated)
The CQ bit was checked by mstensho@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mstensho@opera.com changed reviewers: + thakis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:266: # are building with full debug symbols. this should never happen: https://cs.chromium.org/chromium/src/build/config/sanitizers/sanitizers.gni?q... i know that things explode if you force that on in is msan or ubsan mode at least
https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:266: # are building with full debug symbols. On 2016/12/06 14:34:12, Nico wrote: > this should never happen: > https://cs.chromium.org/chromium/src/build/config/sanitizers/sanitizers.gni?q... I needed it today. I had to debug a use-after free bug. Kind of hard to debug without debug symbols, right?
In which config? Like, the compile doesn't work for me with -g + ubsan On Tue, Dec 6, 2016 at 11:36 AM, <mstensho@opera.com> wrote: > > https://codereview.chromium.org/2553043002/diff/1/build/ > config/sanitizers/BUILD.gn > File build/config/sanitizers/BUILD.gn (right): > > https://codereview.chromium.org/2553043002/diff/1/build/ > config/sanitizers/BUILD.gn#newcode266 > build/config/sanitizers/BUILD.gn:266: # are building with full debug > symbols. > On 2016/12/06 14:34:12, Nico wrote: > > this should never happen: > > > https://cs.chromium.org/chromium/src/build/config/ > sanitizers/sanitizers.gni?q=sanitizers.gni&sq=package:chromium&dr&l=176 > > I needed it today. I had to debug a use-after free bug. Kind of hard to > debug without debug symbols, right? > > https://codereview.chromium.org/2553043002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/06 16:38:00, Nico wrote: > In which config? Like, the compile doesn't work for me with -g + ubsan This was with asan, and it worked fine. Full gn args: is_asan = true is_component_build = true enable_nacl=false cc_wrapper="ccache" gdb_index = true
https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... File build/config/sanitizers/BUILD.gn (right): https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... build/config/sanitizers/BUILD.gn:266: # are building with full debug symbols. On 2016/12/06 16:36:53, mstensho wrote: > On 2016/12/06 14:34:12, Nico wrote: > > this should never happen: > > > https://cs.chromium.org/chromium/src/build/config/sanitizers/sanitizers.gni?q... > > I needed it today. I had to debug a use-after free bug. Kind of hard to debug > without debug symbols, right? So I guess you disabled that assert in the other file, yes? Maybe that comment there should say "Uncomment this locally and also uncomment -gline-tables-only in sanitizers/BUILD.gn" instead? Keying this off is_debug seems wrong (I often build with release+symbols, and I also sometimes do asan-builds with printf debugging where i really just need line tables)
On 2016/12/06 16:47:44, Nico wrote: > https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... > File build/config/sanitizers/BUILD.gn (right): > > https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... > build/config/sanitizers/BUILD.gn:266: # are building with full debug symbols. > On 2016/12/06 16:36:53, mstensho wrote: > > On 2016/12/06 14:34:12, Nico wrote: > > > this should never happen: > > > > > > https://cs.chromium.org/chromium/src/build/config/sanitizers/sanitizers.gni?q... > > > > I needed it today. I had to debug a use-after free bug. Kind of hard to debug > > without debug symbols, right? > > So I guess you disabled that assert in the other file, yes? No, the CL that you have here worked for me, without any assertion failures. > Maybe that comment > there should say "Uncomment this locally and also uncomment -gline-tables-only > in sanitizers/BUILD.gn" instead? Keying this off is_debug seems wrong (I often > build with release+symbols, Ok, so is_debug is the wrong check, perhaps? I basically just want the -g specified - if specified - not to be overridden by -gline-tables-only. > and I also sometimes do asan-builds with printf > debugging where i really just need line tables) Real debugging (with symbols) with asan used to work out of the box, but broke some time ago, either this year or last year. I just want it back. :)
On 2016/12/06 16:54:59, mstensho wrote: > On 2016/12/06 16:47:44, Nico wrote: > > > https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... > > File build/config/sanitizers/BUILD.gn (right): > > > > > https://codereview.chromium.org/2553043002/diff/1/build/config/sanitizers/BUI... > > build/config/sanitizers/BUILD.gn:266: # are building with full debug symbols. > > On 2016/12/06 16:36:53, mstensho wrote: > > > On 2016/12/06 14:34:12, Nico wrote: > > > > this should never happen: > > > > > > > > > > https://cs.chromium.org/chromium/src/build/config/sanitizers/sanitizers.gni?q... > > > > > > I needed it today. I had to debug a use-after free bug. Kind of hard to > debug > > > without debug symbols, right? > > > > So I guess you disabled that assert in the other file, yes? > > No, the CL that you have here worked for me, without any assertion failures. > > > Maybe that comment > > there should say "Uncomment this locally and also uncomment -gline-tables-only > > in sanitizers/BUILD.gn" instead? Keying this off is_debug seems wrong (I often > > build with release+symbols, > > Ok, so is_debug is the wrong check, perhaps? I basically just want the -g > specified - if specified - not to be overridden by -gline-tables-only. > > > and I also sometimes do asan-builds with printf > > debugging where i really just need line tables) > > Real debugging (with symbols) with asan used to work out of the box, but broke > some time ago, either this year or last year. I just want it back. :) Do you know where it broke? (gn switch?)
On 2016/12/06 17:04:28, Nico wrote: > Do you know where it broke? (gn switch?) I'm pretty sure I haven't been able to do this after the gn switch, so that's likely. I also cannot recall having trouble with it when I used gyp, but this doesn't prove anything, of course. :)
ping?
ping
mstensho@opera.com changed reviewers: + ochang@chromium.org
sorry, but this CL not lgtm in its current form. ClusterFuzz (and maybe other things) rely on debug ASan builds, which would probably explode in size with this change. The best solution is probably to just uncomment this line when you need full debug symbols in an ASan build.
On 2017/02/07 16:43:48, Oliver Chang wrote: > sorry, but this CL not lgtm in its current form. > > ClusterFuzz (and maybe other things) rely on debug ASan builds, which would > probably explode in size with this change. > > The best solution is probably to just uncomment this line when you need full > debug symbols in an ASan build. s/uncomment/comment/
Thanks for the feedback. Comments below. On 2017/02/07 16:43:48, Oliver Chang wrote: > sorry, but this CL not lgtm in its current form. > > ClusterFuzz (and maybe other things) rely on debug ASan builds, which would > probably explode in size with this change. There are already non-asan debug builds, and should there be any reason to believe that asan debug builds would be bigger than that? > The best solution is probably to just uncomment this line when you need full > debug symbols in an ASan build. I'm hoping for a solution where I at least won't have to make local changes. I'm merely a simple user of the build system, so I may have got it all wrong, but I must say that I find it confusing that enabling asan destroys debugging symbols when is_debug==true. I also find it equally weird that enabling asan when is_debug==false actually forces line tables to be generated nevertheless. Couldn't there be an additional gn argument instead, e.g. "line_tables_only" (default false) or whatever? So that: is_debug && !line_tables_only -> -g is_debug && line_tables_only -> -gline-tables-only !is_debug && !line_tables_only -> (no debugging) !is_debug && line_tables_only -> (no debugging) If that's unacceptable, a "no_line_tables_only", "full_debug" or whatever (default false) would also work for me (when set, it should prevent -gline-tables-only). But that's hacky and just as confusing as today's solution. Or, even more so, one might argue.
Sorry for the delayed reply. On 2017/02/07 19:49:41, mstensho wrote: > Thanks for the feedback. Comments below. > > On 2017/02/07 16:43:48, Oliver Chang wrote: > > sorry, but this CL not lgtm in its current form. > > > > ClusterFuzz (and maybe other things) rely on debug ASan builds, which would > > probably explode in size with this change. > > There are already non-asan debug builds, and should there be any reason to > believe that asan debug builds would be bigger than that? > > > The best solution is probably to just uncomment this line when you need full > > debug symbols in an ASan build. > > I'm hoping for a solution where I at least won't have to make local changes. > > I'm merely a simple user of the build system, so I may have got it all wrong, > but I must say that I find it confusing that enabling asan destroys debugging > symbols when is_debug==true. I also find it equally weird that enabling asan > when is_debug==false actually forces line tables to be generated nevertheless. > Couldn't there be an additional gn argument instead, e.g. "line_tables_only" > (default false) or whatever? The reason why we always enforce -gline-tables-only when we build ASan in release is because ASan output isn't very useful without it, but agreed that there should be a way to enable full debugging symbols with debug ASan builds. I've filed a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=692620 to track this. > > So that: > > is_debug && !line_tables_only -> -g > is_debug && line_tables_only -> -gline-tables-only > !is_debug && !line_tables_only -> (no debugging) > !is_debug && line_tables_only -> (no debugging) > > If that's unacceptable, a "no_line_tables_only", "full_debug" or whatever > (default false) would also work for me (when set, it should prevent > -gline-tables-only). But that's hacky and just as confusing as today's solution. > Or, even more so, one might argue.
Thanks! I'll close this CL, then. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
