[clang] Add clang::preferred_type attribute for bitfields #69104
Conversation
Endilll
added
clang:frontend
Endilll
requested review from
cor3ntin,
AaronBallman,
Michael137 and
dwblaikie
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-debuginfo Author: Vlad Serebrennikov (Endilll) ChangesThis attribute allows user to specify type of the bitfield that will be emitted to debug info without affecting semantics of the program. Since it doesn't affect semantics, this attribute can be safely ignored by other compilers. This is useful when user is forced to use the same type for all bitfields in a class to get better layout and codegen from MSVC, because it allows debuggers to interpret the value of bitfield in the most human-friendly way (e.g. when value actually comes from an enum). This is driven by my work on LLDB formatters for Clang. I have two use cases for this: namespace Clang {
class Type {
enum TypeClass { ... };
struct TypeBitfields {
[[clang::debug_info_type(clang::Type::TypeClass)]] unsigned TC: 8;
[[clang::debug_info_type(bool)]] mutable unsigned FromAST : 1;
};
};
}Full diff: https://github.com/llvm/llvm-project/pull/69104.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 5c9eb7b8a981037..024421c0583c019 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -107,6 +107,10 @@ def NonBitField : SubsetSubject<Field,
[{!S->isBitField()}],
"non-bit-field non-static data members">;
+def BitField : SubsetSubject<Field,
+ [{S->isBitField()}],
+ "bit-field non-static data members">;
+
def NonStaticCXXMethod : SubsetSubject<CXXMethod,
[{!S->isStatic()}],
"non-static member functions">;
@@ -4264,3 +4268,10 @@ def CountedBy : InheritableAttr {
void setCountedByFieldLoc(SourceRange Loc) { CountedByFieldLoc = Loc; }
}];
}
+
+def DebugInfoType: InheritableAttr {
+ let Spellings = [Clang<"debug_info_type">];
+ let Subjects = SubjectList<[BitField], ErrorDiag>;
+ let Args = [TypeArgument<"Type", 1>];
+ let Documentation = [DebugInfoTypeDocumentation];
+}
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 9f9991bdae36155..6cceba1e0e0ad01 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -7219,6 +7219,18 @@ its underlying representation to be a WebAssembly ``funcref``.
}];
}
+def DebugInfoTypeDocumentation : Documentation {
+ let Category = DocCatField;
+ let Content = [{
+This attribute allows to alter type of a bitfield in debug information.
+Such a need might arise when bitfield is intended to store an enumeration value,
+but has to be specified as having enumeration's underlying type, in order to
+facilitate compiler optimizations. But this also causes underlying type to be
+emitted in debug information, making it hard for debuggers to map bitfield's
+value back to enumeration. This attribute helps with this.
+ }];
+}
+
def CleanupDocs : Documentation {
let Category = DocCatType;
let Content = [{
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index e85cd4d1a1ddc0d..b5c73494df367a6 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3153,6 +3153,8 @@ def err_invalid_branch_protection_spec : Error<
"invalid or misplaced branch protection specification '%0'">;
def warn_unsupported_branch_protection_spec : Warning<
"unsupported branch protection specification '%0'">, InGroup<BranchProtection>;
+def warn_attribute_underlying_type_mismatch : Warning<
+ "underlying type %0 of enumeration %1 doesn't match bitfield type %2">;
def warn_unsupported_target_attribute
: Warning<"%select{unsupported|duplicate|unknown}0%select{| CPU|"
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index c73a63e12f03aab..85aedd87b21d41e 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1497,6 +1497,8 @@ CGDebugInfo::createBitFieldType(const FieldDecl *BitFieldDecl,
llvm::DIScope *RecordTy, const RecordDecl *RD) {
StringRef Name = BitFieldDecl->getName();
QualType Ty = BitFieldDecl->getType();
+ if (BitFieldDecl->hasAttr<DebugInfoTypeAttr>())
+ Ty = BitFieldDecl->getAttr<DebugInfoTypeAttr>()->getType();
SourceLocation Loc = BitFieldDecl->getLocation();
llvm::DIFile *VUnit = getOrCreateFile(Loc);
llvm::DIType *DebugType = getOrCreateType(Ty, VUnit);
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index feb02cad9080e3e..8d58968b7f985c8 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5910,6 +5910,28 @@ static void handleBuiltinAliasAttr(Sema &S, Decl *D,
D->addAttr(::new (S.Context) BuiltinAliasAttr(S.Context, AL, Ident));
}
+static void handleDebugInfoTypeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+ if (!AL.hasParsedType()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;
+ return;
+ }
+
+ TypeSourceInfo *ParmTSI = nullptr;
+ QualType type = S.GetTypeFromParser(AL.getTypeArg(), &ParmTSI);
+ assert(ParmTSI && "no type source info for attribute argument");
+
+ if (type->isEnumeralType()) {
+ QualType BitfieldType = llvm::cast<FieldDecl>(D)->getType();
+ QualType EnumUnderlyingType = type->getAs<EnumType>()->getDecl()->getIntegerType();
+ if (EnumUnderlyingType != BitfieldType) {
+ S.Diag(AL.getLoc(), diag::warn_attribute_underlying_type_mismatch) << EnumUnderlyingType << type << BitfieldType;
+ return;
+ }
+ }
+
+ D->addAttr(::new (S.Context) DebugInfoTypeAttr(S.Context, AL, ParmTSI));
+}
+
//===----------------------------------------------------------------------===//
// Checker-specific attribute handlers.
//===----------------------------------------------------------------------===//
@@ -9629,6 +9651,10 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
handleBuiltinAliasAttr(S, D, AL);
break;
+ case ParsedAttr::AT_DebugInfoType:
+ handleDebugInfoTypeAttr(S, D, AL);
+ break;
+
case ParsedAttr::AT_UsingIfExists:
handleSimpleAttribute<UsingIfExistsAttr>(S, D, AL);
break;
diff --git a/clang/test/CodeGen/debug-info-debug-info-type.cpp b/clang/test/CodeGen/debug-info-debug-info-type.cpp
new file mode 100644
index 000000000000000..4b60d1b64be239f
--- /dev/null
+++ b/clang/test/CodeGen/debug-info-debug-info-type.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang -target x86_64-linux -g -S -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -verify -DMISMATCH %s
+
+struct A {
+ enum E : unsigned {};
+ [[clang::debug_info_type(E)]] unsigned b : 2;
+#ifdef MISMATCH
+ [[clang::debug_info_type(E)]] int b2 : 2;
+ // expected-warning@-1 {{underlying type 'unsigned int' of enumeration 'E' doesn't match bitfield type 'int'}}
+#endif
+} a;
+
+// CHECK-DAG: [[ENUM:![0-9]+]] = !DICompositeType(tag: DW_TAG_enumeration_type, name: "E"{{.*}}
+// CHECK-DAG: !DIDerivedType(tag: DW_TAG_member, name: "b",{{.*}} baseType: [[ENUM]]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Does this issue not apply to other platforms? (I didn't think you could bitfield enums) I'd think maybe rather than adding an attribute to workaround MSVC's missing support - code could #ifdef in a way that's ABI compatible, maybe? (though I admit that's a subtle thing to get right & the cost for getting it wrong is significant) |
As far as I understand, MS ABI forces compilers to insert padding between bitfields more often than Itanium. Probably because of that GCC and Clang are able to merge separate loads of bitfields into single load on Linux.
It's technically possible to Even if we focus on Linux, I'd rather pay with lots of boilerplate in LLDB formatter (lower estimate is 4 lines per every single bitfield), than litter Clang headers with |
There is a deliberate layout/ABI choice that MSVC made ages ago and will never change. It will only pack bitfields into the same word if the neighboring bitfield declarations have the same base type. This is relatively well known. That said, being able to attach an enum (or whatever) to the bitfield does seem handy, for any target. |
|
Seems a bit unfortunate/bit of a heavyweight workaround - but I don't fundamentally object if @AaronBallman or co are OK with the new attribute for this. |
|
Wouldn't it be better to go the other way around? i.e. have a [[clang::compressed_bitfield]] bool IsSomething : 1;
[[clang::compressed_bitfield]] MyEnum Whatever : 3;
[[clang::compressed_bitfield]] int MoreStuff : 4;which the current approach doesn't allow. |
The issue is that MSVC wouldn't know/recognize/implement this attribute - so clang couldn't do that without breaking ABI compatibility with MSVC. |
That's not important for everybody, and you could still #if __has_cpp_attribute(clang::compressed_bitfield)
#define BITFILED_T(ActualType, ABIType) static_assert(check_abi_compat<ActualType, ABIType>()); [[clang::compressed_bitfield]] ActualType
#else
#define BITFIELD_T(ActualType, ABIType) ABIType
...Sure, it's not super pretty, but it gets you there and allows you to do more stuff. We could also ask the MSVC folks whether there would be interest to add such an attribute, since there is clearly interest in having a good ABI and stronger typing (CC @StephanTLavavej @CaseyCarter). |
I think that's a somewhat orthogonal feature to the one @Endilll is proposing. While it might make sense to ask the MSVC and GCC folks if they'd be willing to entertain a similar attribute, the implementation landscape is much wider in C and those implementations may have similar ABI issues. If an implementation ignores the We have a |
While I agree that a 'pack-at-the-bit-level' type attribute might be valuable, I too see that as a 'different' problem. There is plenty of code (even in Clang itself) where and Enum is being stored as a bitfield, and more interesting debugging information could be generated for it. I don't mind the name, I think there IS a nice level of symmetry to One thing I REALLY hate about our pattern of using bitfields for enums, is that it becomes pretty trivial to add an entry to an enum and forget to increase the size of the bitfield. I think such a attribute could cause a diagnostic of So in conclusion, I don't mind the name, but also prefer something more general. |
Couldn't we add a warning for that in general? Seems like a pretty useful one to me. |
Not without SOME sort of psychic-level analysis from Sema. Without some level of mapping of "I mean for this to hold the values of this Enum", we don't have the information to do so. We cannot take advantage of assignments (though a similar warning DOES exist in the 'range' checking) since it might be intended that the field does some level of 'conversion' between enums. |
I meant warning on something like https://godbolt.org/z/Ma17xjjc5. That doesn't seem like it should be that hard. |
I composed an example of that: struct A {
short a1;
short a2;
};
struct B {
[[clang::preferred_type(A)]] unsigned b1 : 32 = 0x000F'000C;
};
int main()
{
B b;
return b.b1;
}Nightly build of LLDB from apt.llvm.org handles it just fine: @erichkeane Would you still prefer restricting type argument to integral and enumeration types? |
An example where the layout doesn't match the normal struct layout might be more interesting? (like the reality of this situation ^ seems indistinguishable from similar code that used the real type and no bitfield, I think? (as far as the DWARF,struct layouts, etc, are concerned)) - maybe a struct with a I would guess a DWARF consumer might have more struggles there - though I Don't think it means we shouldn't do it, just expect to have to fix debuggers for this probably-rather-novel DWARF. |
I'm retracting my suggestion there. I think this should be a default behavior of the debugger, but it shouldn't be forced on users by the compiler. Users who don't expect that behavior then won't have to fight against implicit compiler behavior. |
I think that is the least dangerous, but the |
That's my thinking indeed, and the reason why I opposed to Aaron's proposal to implicitly mark 1-bit bit-fields as |
Suggestion for the release note would be something along these lines: |
|
Thank you @AaronBallman for writing even more documentation! |
There was a problem hiding this comment.
These changes LGTM, but please give @erichkeane a chance to chime in given his previous feedback.
…70349) This patch adds `clang::preferred_type` annotations to Type-related bit-fields. At the moment only debug info takes advantage of this annotation. See more in #69104 This patch also propagates underlying type of several enums from bit-field declaration to enum declaration. I don't see how having them diverge helps.
…(#70349) This patch adds `clang::preferred_type` annotations to Type-related bit-fields. At the moment only debug info takes advantage of this annotation. See more in llvm/llvm-project#69104 This patch also propagates underlying type of several enums from bit-field declaration to enum declaration. I don't see how having them diverge helps.
This attribute allows user to specify type of the bitfield that will be emitted to debug info without affecting semantics of the program. Since it doesn't affect semantics, this attribute can be safely ignored by other compilers.
This is useful when user is forced to use the same type for all bitfields in a class to get better layout and codegen from MSVC, because it allows debuggers to interpret the value of bitfield in the most human-friendly way (e.g. when value actually comes from an enum). This is driven by my work on LLDB formatters for Clang. I have two use cases for this: