[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