clayborg added a comment.
Looks good. Would be nice to add support for byte sizes of 3, 5 and 7 to the
unchecked version as noted in inline comments, or remove the function if no one
is using this function. Just a few quick fixes and this will be good to go.
================
Comment at: source/Utility/DataExtractor.cpp:566
size_t byte_size) const {
- switch (byte_size) {
- case 1:
- return GetU8(offset_ptr);
- break;
- case 2:
- return GetU16(offset_ptr);
- break;
- case 4:
- return GetU32(offset_ptr);
- break;
- default:
- assert(false && "GetMaxU32 unhandled case!");
- break;
- }
- return 0;
+ assert(byte_size <= 4 && "GetMaxU32 unhandled case!");
+ return GetMaxU64(offset_ptr, byte_size);
----------------
petpav01 wrote:
> zturner wrote:
> > jingham wrote:
> > > petpav01 wrote:
> > > > jingham wrote:
> > > > > This is trivial, and you didn't change what was there, but this
> > > > > message makes it sound like this is just something we haven't gotten
> > > > > to yet. It's really "You passed in an illegal byte size"... Might
> > > > > be clearer if the message said that.
> > > > I was not sure what is the expected behaviour when the input
> > > > `byte_size` exceeds the size of the return type of each of these
> > > > `GetMax...()` methods. The current behaviour is to assert this
> > > > situation but comments describing the methods (in both
> > > > `DataExtractor.cpp` and `DataExtractor.h`) say that nothing should get
> > > > extracted in these cases and zero is returned.
> > > >
> > > > Maybe the patch should go a bit further and clean this up as follows:
> > > > * Remove duplicated comments in `DataExtractor.cpp` for
> > > > `DataExtractor::GetMaxU32()` and `GetMaxU64()` and keep only their
> > > > Doxygen versions in `DataExtractor.h`.
> > > > * Update comments in `DataExtractor.h` for
> > > > `DataExtractor::GetMaxU32()`, `GetMaxU64()`, `GetMaxS64()`,
> > > > `GetMaxU64Bitfield()` and `GetMaxS64Bitfield()` to match the current
> > > > implementation.
> > > > * Change assertion text in `DataExtractor::GetMaxU32()` and
> > > > `GetMaxU64()` from "unhandled case" to "invalid byte size".
> > > >
> > > > Does this sound reasonable?
> > > The released versions of lldb - at least the ones Apple releases - have
> > > asserts disabled. This isn't unique to lldb, clang does the same thing.
> > >
> > >
> > > I do my day-to-day debugging using a TOT build with asserts enabled, and
> > > we run the testsuite that way so the asserts catch errors at this stage.
> > > But for the general public, the function will behave as described. It
> > > would be great to remove the duplicated docs - that's just begging for
> > > one or the other to get out of date. But the descriptions are
> > > functionally correct. And then changing the text to "invalid byte size"
> > > also seems good to me.
> > Being pedantic, this *is* a functionality change. Previously, we would
> > assert on a size of 3 or 0, with this change we will allow those cases
> > through.
> To explain myself better, what I was thinking about is that e.g.
> `GetMaxU64()` should have part:
>
> "\a byte_size should have a value greater than or equal to one and less than
> or equal to eight since the return value is 64 bits wide. Any \a byte_size
> values less than 1 or greater than 8 will result in nothing being extracted,
> and zero being returned."
>
> changed to:
>
> "\a byte_size must have a value greater than or equal to one and less than or
> equal to eight since the return value is 64 bits wide. The behaviour is
> undefined for any \a byte_size values less than 1 or greater than 8."
>
> This way the comment provides information that does not depend on whether
> assertions are enabled or not. The behaviour for `byte_size > 8` is said to
> be undefined in the updated description because it either results in an
> assertion failure or some undefined behaviour if asserts are disabled.
>
> If the behaviour for `byte_size > 4/8` with assertions disabled should
> actually be that these methods still return 0 and do not advance the offset
> then the patch has two bugs:
> * The general case added in `GetMaxU64()` is not correct. It returns an
> unexpected value for `byte_size > 8` and advances the offset.
> * `GetMaxU32()` needs to have `if (byte_size > 4) return 0;` added before it
> calls `GetMaxU64()` to avoid the same problem for any `byte_size > 4`.
>
> An additional thing is that the patch causes that `byte_size == 0` is now
> fully valid and does not assert. This might not be the best idea given that
> the current descriptions say that `byte_size` values should be in interval
> [1, 4/8]. I will add the assertion for `byte_size == 0` back in the updated
> patch so the changes affect/enable only `byte_size` in range [1, 4/8] (which
> are clear to be valid) and the zero corner case has its behaviour unchanged.
use lldbassert if the function will function correctly with the assert removed.
I know the previous code was always asserting, but we should change it to use
lldbassert to make sure we don't crash the debugger in release builds.
================
Comment at: source/Utility/DataExtractor.cpp:626
default:
- assert(false && "GetMax64 unhandled case!");
- break;
+ llvm_unreachable("GetMax64_unchecked unhandled case!");
}
----------------
Shouldn't we handle the 3, 5 and 7 sizes here too?
================
Comment at: unittests/Core/DataExtractorTest.cpp:134
+ EXPECT_EQ(8U, offset);
+}
----------------
add a test for the unchecked version here?
https://reviews.llvm.org/D38394
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
