Make -gsource-map independent of name sections and JS minification #25238
Conversation
dschuff
changed the title
add case to ubsan_full_stack_trace tests
|
OK I think this is fully working now. I had to make an update to mark wasm-metadce as a tool that supported source map passthrough (since previously gsource-map blocked metadce). |
|
@aheejin did you want to also review this one before I land it? |
|
|
||
| import common | ||
| from common import BrowserCore, RunnerCore, path_from_root, has_browser, Reporting, is_chrome, is_firefox, CHROMIUM_BASED_BROWSERS | ||
| from common import create_file, parameterized, ensure_dir, disabled, flaky, test_file, WEBIDL_BINDER |
There was a problem hiding this comment.
Is this needed in the test now? If so maybe add a comment why? (I assume the source map is needed for logging, but not sure why -g2)
There was a problem hiding this comment.
The test expects both names and source lines in the output.
There was a problem hiding this comment.
I see, thanks, so -g2 is for the names section.
| (['-O2', '--closure=1', '-g1'], True, True, True), | ||
| (['-O2', '--minify=0'], False, True, False), | ||
| (['-O2', '--profiling-funcs'], True, False, False), | ||
| (['-O2', '--profiling'], False, True, False), |
There was a problem hiding this comment.
Same answer: the test expects both names and line numbers.
| # for clang | ||
| if settings.DEBUG_LEVEL < 3: | ||
| if (settings.DEBUG_LEVEL < 3 and not | ||
| (settings.GENERATE_SOURCE_MAP or settings.SEPARATE_DWARF)): |
There was a problem hiding this comment.
Why does something about settings.SEPARATE_DWARF has to change here? (What about split-dwarf and normal dwarf cases? Don't we need the same DEBUG_LEVEL for them?)
And has settings.SEPARATE_DWARF been set by this point? It looks we parse -gseparate-dwarf in line 385 and set settings.SEPARATE_DWARF in line 394 and 396.
There was a problem hiding this comment.
Yeah, this logic is really tricky. Basically if you have -gsource-map and -g2 together at compile time (to get SM and names), then you set settings.GENERATE_SOURCE_MAP when you parse -gsource-map, and then when you parse -g2 it sets newargs[i] = '-g0' which turns off debug info entirely for the compile and breaks source maps.
Similarly with separate-dwarf which has the side effect of enabling debug info (rather than purely specifying that dwarf should be separated).
This change keeps that from happening, but I also think the fact that -g2 rounds to -g0 at compile time is a bug to fix in a future PR.
There was a problem hiding this comment.
I'm not sure if I understand. And -gsource-map and -gseparate-dwarf are parsed below this line (-gsoure-map in line 399, -gseparate-dwarf in line 385) and settings.GENERATE_SOURCE_MAP and settings.SEPARATE_DWARF are set there, so I'm not sure how we can query these here. If this passes all tests that probably means I'm mistaken though.
Also where do we process -gsplit-dwarf?
There was a problem hiding this comment.
the problem is when you have both flags you make 2 trips through this loop with gsource-map first. the first time it sets DEBUG_LEVEL to 3 and then the second time it sets it back to 2 and sets the 'g0' flag to clang, meaning no debug info at all.
-gsplit-dwarf falls into the else case at line 405.
There was a problem hiding this comment.
Ah thanks, I forgot this code was within a loop...
3 participants
Previously, The
-gsource-mapflag (in addition to generating a source map) caused a name sectionto be generated in the wasm binary, and also suppressed minification of the JS glue.
This PR makes -gsource-map independent of name sections and minification, so it no longer does
either of those. This allows source maps to be used with fully-optimized release builds, for use
cases such as symbolizing stack traces from production.
To get the old behavior, the
-g2flag can be added to-gsource-map.Fixes #20462
Fixes #24626