[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

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.


Repository:
  rC Clang

https://reviews.llvm.org/D51990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

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.


Repository:
  rC Clang

https://reviews.llvm.org/D51990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

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. 


Repository:
  rC Clang

https://reviews.llvm.org/D51990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

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! 


Repository:
  rC Clang

https://reviews.llvm.org/D51990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

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.


Repository:
  rC Clang

https://reviews.llvm.org/D51990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Prantl via Phabricator via cfe-commits]

aprantl requested changes to this revision.
aprantl added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {

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?


Repository:
  rC Clang

https://reviews.llvm.org/D51990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: aprantl, dexonsmith.

We generate incorrect values for the DW_AT_data_bit_offset for interfaces in 
Objective-C. I can only speculate as to what we were trying to achieve by 
taking the modulo of the bit size with the byte size, but given that the size 
and offset is expressed in bits, this seems wrong.


Repository:
  rC Clang

https://reviews.llvm.org/D51990

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenObjC/debug-info-ivars.m


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -15,30 +15,29 @@
 }
 @end
 
-@implementation BaseClass
-@end
+@implementation BaseClass
+@end
 
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
-// CHECK-SAME:   line: 10
-// CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[INT]] = !DIBasicType(name: "int"
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
-// CHECK-SAME:   line: 11
-// CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
-// CHECK-SAME:   line: 12
-// CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
-// CHECK-SAME:   line: 14
-// CHECK-SAME:   baseType: ![[UNSIGNED]]
-// CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
+// CHECK-SAME:   line: 10
+// CHECK-SAME:   baseType: ![[INT:[0-9]+]]
+// CHECK-SAME:   size: 32,
+// CHECK-NOT:offset:
+// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: ![[INT]] = !DIBasicType(name: "int"
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
+// CHECK-SAME:   line: 11
+// CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
+// CHECK-SAME:   size: 9, offset: 96
+// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
+// CHECK-SAME:   line: 12
+// CHECK-SAME:   baseType: ![[UNSIGNED]]
+// CHECK-SAME:   size: 9, offset: 105
+// CHECK-SAME:   flags: DIFlagProtected
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
+// CHECK-SAME:   line: 14
+// CHECK-SAME:   baseType: ![[UNSIGNED]]
+// CHECK-SAME:   size: 9, offset: 115
+// CHECK-SAME:   flags: DIFlagProtected
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2363,13 +2363,10 @@
   // We don't know the runtime offset of an ivar if we're using the
   // non-fragile ABI.  For bitfields, use the bit offset into the first
   // byte of storage of the bitfield.  For other fields, use zero.
-  if (Field->isBitField()) {
-FieldOffset =
-CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field);
-FieldOffset %= CGM.getContext().getCharWidth();
-  } else {
-FieldOffset = 0;
-  }
+  FieldOffset =
+  Field->isBitField()
+  ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field)
+  : 0;
 } else {
   FieldOffset = RL.getFieldOffset(FieldNo);
 }


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -15,30 +15,29 @@
 }
 @end
 
-@implementation BaseClass
-@end
+@implementation BaseClass
+@end
 
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "i"
-// CHECK-SAME:   line: 10
-// CHECK-SAME:   baseType: ![[INT:[0-9]+]]
-// CHECK-SAME:   size: 32,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[INT]] = !DIBasicType(name: "int"
-// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_1"
-// CHECK-SAME:   line: 11
-// CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
-// CHECK-SAME:   size: 9,
-// CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
-// CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned