Two fixes for the -g (debug info) option#6931
Conversation
"Block"s in DWARF are arbitrary-sized constants. They are used for e. g. __int128 constants in C/C++, which is how this was hit.
This is generated by VS when using its "open folder feature".
| let value_labels_ranges = | ||
| self.compute_value_labels_ranges(regalloc, &inst_offsets[..], func_body_len); |
There was a problem hiding this comment.
It is interesting to note that optimize_branches is invoked after this code one last time and can nominally modify the current buffer pointer, make func_body_len stale, etc. At the same time, for it to remove a branch, that branch needs to point past the end of the function, which doesn't seem possible?
There was a problem hiding this comment.
Hmm -- it's theoretically possible, if it were a block that had just a jump instruction, no fallthrough (previous block ended in an uncond branch), and we rewrote branches to this last block to go directly to the jump dest instead.
Perhaps you could pull out the result of buffer.finish(...) (which is where the last optimization call occurs) to before this line?
SingleAccretion
changed the title
-g (DWARF debug info) option-g (debug info) option
Turns out that this is easier then fixing them in-place when removing the branch because a bunch of ambient state needs to be passed through to the MachBuffer for that to become possible. Testing: tested to fix the issue on both one of the reported samples as well as my own.
SingleAccretion
force-pushed
the
DI-Fixes
branch
from
d4dd52e to
2fe2fd4
Compare
github-actions
Bot
added
cranelift
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Looks good to me!
Would it be possible to construct a somewhat minimal test case where the branch lowering messes up the offsets and we need to monotize them and add it to our debugging tests? Eg https://github.com/bytecodealliance/wasmtime/blob/main/tests/all/debug/lldb.rs#L62
| let value_labels_ranges = | ||
| self.compute_value_labels_ranges(regalloc, &inst_offsets[..], func_body_len); |
There was a problem hiding this comment.
Hmm -- it's theoretically possible, if it were a block that had just a jump instruction, no fallthrough (previous block ended in an uncond branch), and we rewrote branches to this last block to go directly to the jump dest instead.
Perhaps you could pull out the result of buffer.finish(...) (which is where the last optimization call occurs) to before this line?
Should definitely be possible; will do. |
It is more obviously correct this way that the code which is using buffer.cur_offset() is not reading stale values.
Also delete EmitResult::inst_offsets, it was not used.
If either is unknown, err on the safe side.
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Latest changes LGTM to me -- thanks very much for this!
* Fix up a debug info transform bug "Block"s in DWARF are arbitrary-sized constants. They are used for e. g. __int128 constants in C/C++, which is how this was hit. * Add the ".vs" folder to ".gitignore" This is generated by VS when using its "open folder feature". * Monotonize instructon offsets after code emission Turns out that this is easier then fixing them in-place when removing the branch because a bunch of ambient state needs to be passed through to the MachBuffer for that to become possible. Testing: tested to fix the issue on both one of the reported samples as well as my own. * Move the last optimize_branches earlier It is more obviously correct this way that the code which is using buffer.cur_offset() is not reading stale values. * Fix the zero-offset problem with NO_INST_OFFSET Also delete EmitResult::inst_offsets, it was not used. * Add a test * Check both sides of the range for NO_INST_OFFSET If either is unknown, err on the safe side.
3 participants
Two changes:
See also the commit messages for a bit more detail.
With these two changes, I am able to step through the code generated by the compiler we are working on.
Fixes #3999.