This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Fix getSLEB128 on large values
ClosedPublic

Authored by rprichard on Jul 13 2020, 10:06 PM.

Details

Reviewers
mstorsjo
Group Reviewers
Restricted Project
Commits
rGfd802cc4dea4: [libunwind] Fix getSLEB128 on large values
Summary

Previously, for large-enough values, getSLEB128 would attempt to shift
a signed int in the range [0..0x7f] by 28, 35, 42... bits, which is
undefined behavior and likely to fail.

Avoid shifting (-1ULL) by 70 for large values. e.g. For INT64_MAX, the
last two bytes will be:

  • 0x7f [bit==56]
  • 0x00 [bit==63]

Diff Detail

Event Timeline

rprichard created this revision.Jul 13 2020, 10:06 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 13 2020, 10:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

The shift for negative numbers is also broken.

rprichard edited the summary of this revision. (Show Details)Jul 13 2020, 10:20 PM
mstorsjo accepted this revision.Jul 13 2020, 10:30 PM
mstorsjo added a subscriber: mstorsjo.

LGTM

This revision is now accepted and ready to land.Jul 13 2020, 10:30 PM
This revision was automatically updated to reflect the committed changes.

Would it make sense to pick this to 11.0? CC @hans

hans added a comment.Jul 16 2020, 7:48 AM

Would it make sense to pick this to 11.0? CC @hans

Sounds good to me. Please go ahead and cherry-pick with the -x option, or let me know if you'd prefer me to do it.

Would it make sense to pick this to 11.

Would it make sense to pick this to 11.0? CC @hans

Sounds good to me. Please go ahead and cherry-pick with the -x option, or let me know if you'd prefer me to do it.

Sorry, I wasn't sure if this was directed to me or Ryan (the diff author). I'd be happy to cherry-pick, but I don't have a way of testing it.

hans added a comment.Jul 21 2020, 7:09 AM

Sorry, I wasn't sure if this was directed to me or Ryan (the diff author). I'd be happy to cherry-pick, but I don't have a way of testing it.

I meant you, since you brought it up :-) But if there's no pressing need we can also just let it wait for the next release.

AFAIK, there's no regression, and no one has hit this issue in practice, so maybe it can wait until the next release? It's probably only an issue when the size of something exceeds 2**32?

(I'm more worried about https://bugs.llvm.org/show_bug.cgi?id=46743, because that is a regression.)