[PATCH] D20435: [codeview] Adding support for CodeView types
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu May 19 09:25:42 PDT 2016
dblaikie added a comment.
Here are some basic thoughts - I'd probably wait for Reid to chime in before getting too deep into all this feedback. He might have some higher level feedback, outstanding patches, etc, to worry about.
================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:880
@@ -879,1 +879,3 @@
+ static uint32_t getLayoutSize() { return 2 + sizeof(Layout); }
+
----------------
zturner wrote:
> This looks wrong to me. It does not include the length of the name. Also, I would prefer if the `2 + ` were raised somehow, because it is common to every single type of CodeView record, and this logic of knowing that a type record has 2 header bytes should be common somewhere.
>
> Also, each type record knows how to deserialize itself (see the `deserialize` method above), should we centralize the serialization logic here as well using a similar pattern?
Where's the 2 come from? Could use a comment.
& if this size is being used as part of the output format, you'd probably need to make sure the Layout struct is packed, etc. But might be better not to rely on that at all, if you can help it.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:257
@@ -302,10 +256,3 @@
- OS.AddComment("Scope type index");
- OS.EmitIntValue(0, 4);
- OS.AddComment("Function type");
- OS.EmitIntValue(VoidFnTyIdx, 4);
- {
- OS.AddComment("Function name");
- emitNullTerminatedSymbolName(OS, DisplayName);
- }
- OS.EmitLabel(FuncEnd);
+ for (const auto *CVType : TypeTable) {
+ emitType(CVType);
----------------
We usually skip {} on single line blocks (general code review feedback, not just this instance)
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:874-881
@@ +873,10 @@
+ if (VoidType == nullptr) {
+ CodeViewTypeBasic *CVType;
+
+ CVType = CodeViewTypeBasic::create();
+ CVType->setTypeIndex(TypeIndex::Void());
+ CVType->setSizeInBits(0);
+
+ appendType(CVType);
+ VoidType = CVType;
+ }
----------------
Looks like you could skip the CVType variable and just use VoidType directly. (& I might consider inverting the condition/returning early to reduce indentation)
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:902
@@ +901,3 @@
+
+ if (CVType = getMappedCodeViewType(Type)) {
+ return CVType;
----------------
majnemer wrote:
> I'd just do:
>
> ```
> if (CodeViewType *CVType = getMappedCodeViewType(Type))
> return CVType;
>
> switch (Tag) {
> case dwarf::DW_TAG_base_type:
> return lowerTypeBasic(cast<DIBasicType>(Type));
> case dwarf::DW_TAG_subroutine_type:
> return lowerTypeSubroutine(cast<DISubroutineType>(Type));
> default:
> // TODO: Enable this assert once all types are implemented
> //assert(Tag != Tag && "Unhandled type Tag!");
> return nullptr;
> }
> ```
Sink the variable to its declaration (& that'll also avoid a -Wparentheses warning here)
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:906-907
@@ +905,4 @@
+
+ Tag = Type->getTag();
+ switch (Tag) {
+ case dwarf::DW_TAG_base_type:
----------------
Sink the variable declaration into the switch condition. Oh, then the variable would be unused - remove the variable and just "switch (Type->getTag())"
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:931-932
@@ +930,4 @@
+CodeViewType *CodeViewDebug::lowerTypeBasic(const DIBasicType *Type) {
+ const uint32_t SizeInBits = Type->getSizeInBits();
+ const TypeIndex Index = toBasicTypeIndex(Type);
+ CodeViewTypeBasic *CVType;
----------------
Probably just sink these two variables into their only use - their initialization expressions are pretty self explanatory.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:956-957
@@ +955,4 @@
+ DITypeRefArray Elements, uint16_t FirstArgIndex) {
+ CodeViewTypeArgumentList *ArgList;
+ DIType *ArgType;
+
----------------
I don't /think/ we usually do this sort of alignment, but I could be wrong.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:970-971
@@ +969,4 @@
+ ArgList->append(lowerType(ArgType));
+ }
+ else {
+ assert(i == size - 1); // A variadic argument must be the last argument.
----------------
Formatting ("} else {"). Probably just run the whole thing through clang-format.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1004
@@ +1003,3 @@
+ Arguments = Type->getTypeArray();
+ ReturnType = lowerType(Arguments.size() ? resolve(Arguments[0]) : nullptr);
+
----------------
Prefer !empty over size in this context
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1040-1042
@@ +1039,5 @@
+CodeViewType *CodeViewDebug::lowerSubprogramType(const DISubprogram *SP) {
+ CodeViewType *CVType;
+ const DISubroutineType *Type;
+ const DIType *ClassType;
+
----------------
Move declarations to their first use or otherwise as close as possible (just above the if/else for CVType, for example). Reduces the scope/makes it easier to understand their use, etc. (general feedback, not just this instance)
Actually you could probably skip CVType entirely and just return:
if (...)
return nullptr;
return lowerType(Type);
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1048-1050
@@ +1047,5 @@
+ Type = SP->getType();
+ ClassType = dyn_cast<DIType>(resolve(SP->getScope()));
+
+ if (ClassType) {
+ // TODO: implement this case.
----------------
Then roll this declaration into the if:
if (const auto *ClassType = dyn_cast<DIType>(...))
Oh, the variable's unused - use isa<DIType> instead.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1079-1082
@@ +1078,6 @@
+ if (FuncName.empty())
+ if (GV)
+ FuncName = GlobalValue::getRealLinkageName(GV->getName());
+ else
+ FuncName = SP->getName();
+
----------------
Maybe use the conditional operator here
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1122
@@ +1121,3 @@
+ NullTerminatedString.push_back('\0');
+ OS.EmitBytes(NullTerminatedString);
+}
----------------
Probably more efficient to emit the original bytes, then emit a null byte - rather than copying the whole string, adding a null terminator, then emitting that.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1126-1127
@@ +1125,4 @@
+static size_t paddingBytes(const size_t Size) {
+ const uint32_t Alignment = 4;
+ return (Alignment - (Size % Alignment)) % Alignment;
+}
----------------
I think we have some utility functions for computing teh nearest alignment, which might make this easier to read.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1182-1189
@@ +1181,10 @@
+void CodeViewDebug::emitTypeFunctionID(const CodeViewTypeFunctionID *CVType) {
+ StringRef Name = CVType->getName();
+ const CodeViewType *FuncType = CVType->getType();
+ const CodeViewType *ClassType = CVType->getParentClassType();
+ const CodeViewType *ScopeType =
+ ClassType ? ClassType : CVType->getParentScope();
+ unsigned SymbolID = LF_FUNC_ID;
+ const size_t Length = 2 + FuncIdRecord::getLayoutSize() + Name.size() + 1;
+ const size_t Padding = paddingBytes(Length);
+
----------------
Not sure if this is idiomatic of other parts of the file - but mostly I'd just sink all these variables into their only use (especially since there's the "addComment" line above each emission to indicate what's being emitted in case the expression's not self documenting enough. (so adding a variable name doesn't necessarily add lots of value)
Goes for the other emit functions below.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.h:108
@@ +107,3 @@
+ // All types in the order they should be emitted.
+ typedef std::vector<CodeViewType *> CodeViewTypeTable;
+ CodeViewTypeTable TypeTable;
----------------
Could you use a MapVector to keep the vector and map together. Or, alternatively, to save space, you could create a temporary vector at the point of emission, and sort based on the type id.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.h:152-154
@@ -156,1 +151,5 @@
FileToFilepathMap.clear();
+ for (auto T : TypeTable) {
+ delete T;
+ }
+ TypeTable.clear();
----------------
Should TypeTable contain unique_ptrs instead of raw pointers - then this manual memory management wouldn't be needed?
If it is needed, you can use DeleteContainerPointers to do this work.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewType.cpp:1
@@ +1,2 @@
+//===-- llvm/lib/CodeGen/AsmPrinter/CodeViewType.cpp --*- C++ -*--===//
+//
----------------
Pretty much all the functions in this file are trivial enough to be defined inline in the header.
Though the virtual dtors do need to be out of line for the weak vtable style guide requirement/recommendation.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewType.h:47
@@ +46,3 @@
+public:
+ virtual ~CodeViewType();
+
----------------
Any reason for this to be a virtual hierarchy if it has no other virtual functions? Should it have some?
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewType.h:65
@@ +64,3 @@
+class CodeViewTypeBasic : public CodeViewType {
+protected:
+ CodeViewTypeBasic();
----------------
Seems weird for this to be protected. Nothing else is derived from this type so maybe make it private? (same point for other CodeViewType derived classes)
Also, the naming scheme might be beter as "CodeView<X>Type" rather than "CodeViewType<X>" (eg: CodeViewBasicType here)
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewType.h:153
@@ +152,3 @@
+public:
+ ~CodeViewTypeArgumentList();
+
----------------
Please use 'override' where applicable.
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewType.h:157-158
@@ +156,4 @@
+
+ const ArgTypeList *getArgumentList() const;
+ ArgTypeList *getArgumentList();
+
----------------
Should return by reference, not pointer, I think?
================
Comment at: lib/CodeGen/AsmPrinter/CodeViewType.h:160
@@ +159,3 @@
+
+ uint32_t getArgumentCount() const;
+ void append(CodeViewType *Argument);
----------------
Probably skip this if it just returns getArgumentList().size() ?
================
Comment at: test/DebugInfo/COFF/basic-types.ll:20-21
@@ +19,4 @@
+; 14: unsigned long ul;
+; 15: x += (int)&b + (int)&c + (int)&s + (int)&i + (int)&l + (int)&f + (int)&d
+; 16: + (int)&ld + (int)&uc + (int)&us + (int)&ui + (int)&ul;
+; 17: }
----------------
Pretty sure you don't need uses of variables to cause them to be emitted at -O0, right?
================
Comment at: test/DebugInfo/COFF/function-type.ll:6-8
@@ +5,5 @@
+; 'clang -O0 -c -g -gcodeview -S -emit-llvm' on the following code:
+; 1: int foo(char c, short s, long l, float f) {
+; 2: return 0;
+; 3: }
+
----------------
There's a function type you could test in the basic-types test case, perhaps? (not a hard requirement - it's a tricky tradeoff between multiple mini tests in a single test case, versus pulling things out into entirely separate test cases to make it simpler/more obvious)
http://reviews.llvm.org/D20435
More information about the llvm-commits
mailing list