Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore missing CUs in DWARF rewriting #3700

Merged
merged 6 commits into from
Mar 18, 2021
Merged

Ignore missing CUs in DWARF rewriting #3700

merged 6 commits into from
Mar 18, 2021

Conversation

kripken
Copy link
Member

@kripken kripken commented Mar 18, 2021

A recent change in LLVM causes it to sometimes end up with a thing
with no parent. That is, a debug_line or a debug_loc that has no CU that
refers to it. This is perhaps LLVM DCEing CUs, or something else that
changed - not sure. But it seems like valid DWARF we should handle.

This PR handles that in our code. Two things broke here. First, locs must
be simply ignored when there is no CU. Second, lines are trickier as we
used to compute their position by scanning them, and that list contained
only ones with a CU. So we missed some and ended up with wrong offsets.
To make things simpler and more robust, just track the position of each
line table on itself.

Fixes #3697

@kripken kripken requested review from dschuff and aardappel March 18, 2021 02:18
@@ -151,6 +151,7 @@ struct LineTableOpcode {
};

struct LineTable {
uint64_t Position; // XXX BINARYEN: the binary location in .debug_line
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is set but not used anywhere in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used here: https://github.com/WebAssembly/binaryen/pull/3700/files#diff-62256ba16b71cbddf493d990a92cc71e51a3f2ae0a57289ae85ae5d794db3d31R765

We set this during load. During rewrite, we read these values so we know the old position. (We also update it there, although we do not currently read at any later time - I added that to avoid future confusion, seems best to the keep the IR consistent.)

@kripken kripken merged commit 647ef50 into main Mar 18, 2021
@kripken kripken deleted the line branch March 18, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash in wasm-opt at -O2
2 participants