[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+              ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+              : 0;
     } else {
----------------
JDevlieghere wrote:
> aprantl wrote:
> > JDevlieghere wrote:
> > > JDevlieghere wrote:
> > > > aprantl wrote:
> > > > > aprantl wrote:
> > > > > > It might help to attempt some git blame archeology.
> > > > > > Judging from the comment, it sounds like the debugger is supposed to query the runtime for the *byte* offset and then add the bit offset from DWARF? Could that make sense?
> > > > > If that is the case, we'd need to relax llvm-dwarfdump --verify to accept this and make sure LLDB does the right thing instead.
> > > > Ah I see, yeah that sounds reasonable and explains the comment which I interpreted differently. Thanks! 
> > > btw it was added in rL167503. 
> > We should check whether emitting the offsets like this violates the DWARF spec. If yes, it may be better to emit the static offsets like you are doing here and then still have LLDB ignore everything but the bit-offsets from the dynamic byte offset.
> The standard says 
> 
> > The member entry corresponding to a data member that is defined in a structure,
> > union or class may have either a DW_AT_data_member_location attribute or a
> > DW_AT_data_bit_offset attribute.
> 
> which to me sounds like they should be mutually exclusive. I ran the lldb test suite with my change and there were no new failures, which leads me to believe that the comment from r167503 still holds and lldb ignores this attribute, at least for Objective-C.
In Clang's current code-generation, the ivar offset variable stores the offset of the byte containing the first bit of the bit-field, which is not necessarily the same as the offset of the byte at which the bit-field's storage begins.  This is why the debug info `%`s by the number of bits in a `char`: it's the correct bit-offset for computing where the bit-field begins relative to the value in the ivar offset variable.  See the behavior of `CGObjCRuntime::EmitValueForIvarAtOffset`, which does the same `%` when computing the layout.

It seems right to me that the `%` is done consistently within the compiler, rather than expecting LLDB to do it.  That's the more future-proof design; for example, it would allow the compiler to emit a single ObjC ivar record for the combined bit-field storage and then emit debug info for the bit-fields at their appropriate relative bit-offsets, which could then be `>= 8`.  This would arguably be a better code-generation scheme anyway, for a number of reasons — at the cost of breaking reflective access to the individual ivars, which I don't believe the runtime really supports anyway.

Anyway, the upshot is that I don't think this patch is correct.


Repository:
  rC Clang

https://reviews.llvm.org/D51990