[Lldb-commits] [PATCH] D138197: [lldb] Fix bitfield incorrectly printing when field crosses a storage unit

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Thanks for working on this. Due to llvm dev and vacation it took me a while to get to this.



================
Comment at: lldb/test/API/commands/expression/expr-bitfield-on-boundary/Makefile:3
+
+CXXFLAGS_EXTRAS := -gdwarf-2
+
----------------
Add a comment here to explain why we need dwarf v2 specifically.

Which I think is because in v4 the encoding is done such that you won't get a negative offset?


================
Comment at: lldb/test/API/commands/expression/expr-bitfield-on-boundary/main.cpp:3
+  unsigned char a : 7;
+  unsigned char b : 4;
+};
----------------
Comment here that b is going across the byte boundary.

Maybe on the packed too, iirc without packed it would assign one whole byte to each of a and b.


================
Comment at: lldb/test/API/commands/expression/expr-bitfield-on-boundary/main.cpp:9
+  f.a = 1;
+  f.b = 2;
+  return 0; // Break here
----------------
`b` is going to be across the boundary, should we use a value that is also across the boundary?

Something like 9, so you have `0b1001`. One bit on both sides of the boundary. Just in case lldb actually is dropping one side of the value and we aren't checking for that.


================
Comment at: lldb/unittests/Platform/PlatformSiginfoTest.cpp:62
     for (auto field_name : llvm::split(path, '.')) {
-      uint64_t bit_offset;
       ASSERT_NE(field_type.GetIndexOfFieldWithName(field_name.str().c_str(),
----------------
Are you reasonably sure you found all these cases? Don't want to loose the sign because of an implicit cast.

If not, `-Wconversion` is worth a go though lldb probably has hundreds of those already. Perhaps compare the number of those warnings before and after this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138197/new/

https://reviews.llvm.org/D138197