[PATCH] Add support for dumping fields of interest from function info subsections required to fix PR21189

Timur Iskhodzhanov timurrrr at google.com
Tue Oct 7 10:25:24 PDT 2014


================
Comment at: include/llvm/Support/COFF.h:672
@@ +671,3 @@
+  enum CodeViewDebugSymbolIdentifiers : uint16_t {
+    DEBUG_SYM_ID_FUNC_INFO_START = 0x1147, ///< Function information start
+    DEBUG_SYM_ID_FUNC_INFO_END   = 0x114F  ///< End of function information
----------------
Is this name used somewhere in MSDN or one you've just came up with it?
If it's the latter, I suggest we use `DEBUG_SYMBOL_PROCEDURE_START`

================
Comment at: include/llvm/Support/COFF.h:673
@@ +672,3 @@
+    DEBUG_SYM_ID_FUNC_INFO_START = 0x1147, ///< Function information start
+    DEBUG_SYM_ID_FUNC_INFO_END   = 0x114F  ///< End of function information
+  };
----------------
ditto

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:440
@@ +439,3 @@
+  typedef struct {
+    uint32_t ParentFunc;
+    uint32_t LastFunc;
----------------
I don't think we need all these fields.

I suggest we avoid adding them to the structure as:
a) they are not needed
b) we are not 100% sure this is the actual format of the subsection – and we don't really care yet
c) it adds extra code nobody uses (=> untested)

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:544
@@ +543,3 @@
+        bool FuncScope = false;
+        DataExtractor EX(Contents, true, 4);
+        for (uint32_t WorkOffs = 0; WorkOffs < PayloadSize;) {
----------------
This looks too complicated.

Can you extract this into a separate function and use `DE` and `Offset` in place of `EX` (?) and `WorkOffs` (??) ?

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:547
@@ +546,3 @@
+          FuncInfo Info;
+          StringRef Name;
+          uint16_t Size = EX.getU16(&WorkOffs) - 2;
----------------
The scope of this `Name` variable is too wide.  It's only used in one switch case.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:557
@@ +556,3 @@
+            FuncScope = true;
+            Info.ParentFunc   = EX.getU32(&WorkOffs);
+            Info.LastFunc     = EX.getU32(&WorkOffs);
----------------
See the comment I wrote for `struct` above.

I think it's fine to call `getU32()` where unneeded/uninvestigated fields are located to and just ignore the return value.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:655
@@ +654,3 @@
+
+  // Dump function info
+  for (unsigned I = 0, E = FunctionInfoNames.size(); I != E; ++I) {
----------------
Do we need to dump this separately?  Or can we dump this 80 lines of code earlier? (in the middle of the switch case)

http://reviews.llvm.org/D5652






More information about the llvm-commits mailing list