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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

See inlined comments.



================
Comment at: source/Core/DumpDataExtractor.cpp:275-281
+      // Reject invalid item_byte_size.
+      if (item_byte_size > 8) {
+        s->Printf("error: unsupported byte size (%" PRIu64 ") for char format",
+                  (uint64_t)item_byte_size);
+        return offset;
+      }
+
----------------
petpav01 wrote:
> jingham wrote:
> > Should this consume the weird input we couldn't print?  I actually don't have a good feel for which would be better.
> The behaviour implemented in `DumpDataExtractor()` for other formats, such as `eFormatBoolean` or `eFormatComplexInteger`, is to report an error and do not advance the offset. The approach that the patch takes is to make `eFormatChar` (and its variants) consistent with this behaviour.
The doc's say an error in the size "will result in nothing being extracted".  That seems like a roundabout way of saying that offset won't be advanced, but I probably should have figured it out from there.  Thanks for checking on the error behavior of the other dumpers!  You are doing the right thing.



================
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:
> 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.


https://reviews.llvm.org/D38394