[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC
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
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
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
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
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
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
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
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
