petpav01 added inline comments.
================
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);
----------------
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.
https://reviews.llvm.org/D38394
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
