[Lldb-commits] [PATCH] D38394: Fix dumping of characters with non-standard sizes

[Greg Clayton via Phabricator via lldb-commits]

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