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

Fix off-by-one error in DWARF reg-reg location (#317) #321

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

agocke
Copy link
Member

@agocke agocke commented Nov 17, 2022

Backport of #317

Customer impact

Without this change, symbols are broken on Linux release builds such that no debugging is functional. Found in both manual testing and customer reported soon after shipping.

Testing

Manual testing. Verified using llvm-dwarfdump and gdb, and verified no Windows regression using Visual Studio.

Risk

Low. This is a focused change, and Windows has been scouted for regressions. It's probably not possible to make Linux worse than it already is.

The DWARF specification states that the form of an exprloc consists
of an unsigned LEB128 length value, followed by the encoded location bytes
of the specified length. For some reason we were adding one to the length
value being emitted. This looks incorrect to me. The above calculation for
REG-REG (a variable stored in two registers) correctly calculates the length
of each register type tag, plus the size of the interpolating PIECE tags,
plus the size of notation for each register. The extra byte looks wrong.

I've tested this locally and it appears to resolve dotnet/runtime#77407.

Unfortunately, it also causes llvm-dwarfdump --verify to constantly
complain about missing base addresses. I can't confirm at the moment,
but my suspicion is that this is revealing an existing bug. Even if this
is somehow causing a new bug, I think the resulting symbols with this
change are better than the alternative (no working symbols at all).

(cherry picked from commit b85b64b)
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we should take for consideration in 7.0.x

@agocke agocke merged commit 8cb919f into objwriter/release/7.0 Nov 22, 2022
@agocke agocke deleted the fix-off-by-one branch November 22, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants