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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
