[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Yeah I got confused by the description.

So, do we need to do any work at all with the bitfields offsets? With this patch we just keep adding them with whatever bogus bit-offset we get from DWARF and we just disable the sanity check. If the values are anyway useless, we might as well skip all the work and just add the field to the record? Also there is still the question why the calculations below are based on Obj-C bitfield offsets, which means that code is also calculating some bogus values? And then there is the question why we even have any offsets for this in DWARF when they are wrong. I guess there is a bunch of stuff that needs to be fixed here but this patch makes the code at least better working than before, so it probably should go in.

I would say we just early-exit for Obj-C bitfields from that code or something like that, but that can all be done in a follow-up PR. FWIW, this patch seems mostly ready to go beside Adrian's suggestions and one minor change in the test, so LGTM module comments.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2612
 
-            if ((this_field_info.bit_offset >= parent_bit_size) ||
-                (last_field_info.IsBitfield() &&
-                 !last_field_info.NextBitfieldOffsetIsValid(
-                     this_field_info.bit_offset))) {
+            if (class_language != eLanguageTypeObjC &&
+                ((this_field_info.bit_offset >= parent_bit_size) ||
----------------
aprantl wrote:
> shafik wrote:
> > aprantl wrote:
> > > I think we should add a comment here explaining why this doesn't apply to Objective-C. Also, what about a C struct in Objective-C, or a C++ class in Objective-C++?
> > This is a great point! I think this check will probably be better:
> > 
> > ```
> > !TypeSystemClang::IsObjCObjectOrInterfaceType(class_clang_type)
> > ```
> > 
> > Let me try it out.
> That sounds right to me.
FWIW, I think the original check was also correct. The `class_language` is from what I remember either `Unknown` or `ObjC` (when `IsObjCObjectOrInterfaceType` is true). But `!TypeSystemClang::IsObjCObjectOrInterfaceType(class_clang_type)` seems more expressive.


================
Comment at: lldb/test/API/lang/objc/bitfield_ivars/TestBitfieldIvars.py:10
+
+    def test(self):
+        self.build()
----------------
(the skipUnlessDarwin slipped out of this method and the one below)


================
Comment at: lldb/test/API/lang/objc/bitfield_ivars/main.m:39
     ContainsAHasBitfield *chb = [[ContainsAHasBitfield alloc] init];
-    printf("%d\n", chb->hb->field2); //% self.expect("expression -- chb->hb->field1", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["= 0"])
-                                     //% self.expect("expression -- chb->hb->field2", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["= 1"]) # this must happen second
----------------
Thanks for modernizing this!


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

https://reviews.llvm.org/D83433