Page MenuHomePhabricator

Avoid an assertion failure when a bit field is extracted from a value of the same size.
ClosedPublic

Authored by bryanpkc on May 18 2016, 2:17 AM.

Details

Summary

One of the cases handled by ValueObjectChild::UpdateValue() uses the entire width of the parent's scalar value as the size of the child, and extracts the child by calling Scalar::ExtractBitfield(). This seems valid but APInt::trunc(), APInt::sext() and APInt::zext() assert that the bit field must not have the same size as the parent scalar. Replacing those calls with sextOrTrunc(), zextOrTrunc(), sextOrSelf() and zextOrSelf() fixes the assertion failures.

Diff Detail

Event Timeline

bryanpkc updated this revision to Diff 57581.May 18 2016, 2:17 AM
bryanpkc retitled this revision from to Avoid an assertion failure when a bit field is extracted from a value of the same size..
bryanpkc updated this object.
bryanpkc added a reviewer: uweigand.
bryanpkc added a subscriber: lldb-commits.
labath added a subscriber: labath.

This fixes the same issue as D19535, which is stalled at the moment waiting for a unit test. You can take over if you write a quick unit test for this (unittests/Core/ScalarTest.cpp). Thanks.

bryanpkc updated this revision to Diff 57695.May 18 2016, 3:47 PM

Added a unit test in ScalarTest.cpp that catches this particular error.

labath accepted this revision.May 19 2016, 4:07 AM
labath edited edge metadata.

Looks good. Thank you for taking the time to add the tests. Please consider the comment below though.

Do you have commit access?

unittests/Core/ScalarTest.cpp
90

Is there a reason this couldn't be written as ASSERT_EQ(a1, s_scalar.SLongLong()) ?
If there isn't one, I think this would make the check more readable.

This revision is now accepted and ready to land.May 19 2016, 4:07 AM

Do you have commit access?

Yes I do. Thanks for your review!

unittests/Core/ScalarTest.cpp
90

SLongLong() invokes sextOrTrunc() and getSExtValue(), potentially further changing the contents of the underlying m_integer. I felt that checking with memcmp() immediately after ExtractBitfield() is a more fool-proof way to confirm the behaviour of ExtractBitfield(). There are precedents for using memcmp() in the file, and it is not that much more unreadable.

labath added inline comments.May 19 2016, 6:25 AM
unittests/Core/ScalarTest.cpp
90

Ok, I'll leave that up to you.

bryanpkc closed this revision.May 19 2016, 6:57 AM