[PATCH] D36426: [PDB] Fix linking of function symbols and local variables

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lld/COFF/PDB.cpp:334
+/// MSVC translates S_PROC_ID_END to S_END, and S_[LG]PROC32_ID to S_[LG]PROC32
+static SymbolKind canonicalizeSymbolKind(MutableArrayRef<uint8_t> &RecordData,
+                                         const TypeTableBuilder &IDTable) {
----------------
This thing isn't really canonicalizing the symbol kind anymore, it's rewriting the symbol record. Maybe name it something like `rewriteSymbolForPDB`?


================
Comment at: lld/COFF/PDB.cpp:361
+    if (!TI->isSimple() && !TI->isNoneType()) {
+      ArrayRef<uint8_t> FuncIdData = IDTable.records()[TI->toArrayIndex()];
+      FuncIdRecord FID;
----------------
We need an error check for OOB IPI index


================
Comment at: lld/COFF/PDB.cpp:362-365
+      FuncIdRecord FID;
+      CVType FuncId(TypeLeafKind::LF_FUNC_ID, FuncIdData);
+      cantFail(TypeDeserializer::deserializeAs<FuncIdRecord>(FuncId, FID));
+      *TI = FID.FunctionType;
----------------
In theory, we could do the same `discoverTypeIndices` trick that we do above to find the type index reference. Alternatively, if we know it's an `LF_FUNC_ID`, we should load the appropriate offset. Are these ever LF_MFUNC_IDs, though?


================
Comment at: lld/COFF/PDB.cpp:488
+    remapTypesInSymbolRecord(File, Contents, IndexMap, IDTable, TypeRefs);
+    SymbolKind NewKind = canonicalizeSymbolKind(NewData, IDTable);
+    assert(NewKind == symbolKind(NewData));
----------------
Let's keep this as part of the copySymbolForPdb.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeIndexDiscovery.cpp:401
     break;
   case SymbolKind::S_REGREL32:
     Refs.push_back({TiRefKind::TypeRef, 4, 1}); // Type
----------------
Maybe add S_BPREL32 before this? They're related, BPRELSYM32 vs REGREL32.


================
Comment at: llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp:740
                Proc.CodeSize);
   // FIXME: It seems FunctionType is sometimes an id and sometimes a type.
+  bool IsType = true;
----------------
Remove the FIXME, it's done now. :)


https://reviews.llvm.org/D36426