[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:365
+      CVType FuncId(TypeLeafKind::LF_FUNC_ID, FuncIdData);
+      cantFail(TypeDeserializer::deserializeAs<FuncIdRecord>(FuncId, FID));
+      *TI = FID.FunctionType;
----------------
These can be `LF_MFUNC_ID` records, we need to handle that.


================
Comment at: lld/COFF/PDB.cpp:489
+    SymbolKind NewKind = canonicalizeSymbolKind(NewData, IDTable);
+    assert(NewKind == symbolKind(NewData));
+    assert(NewKind != SymbolKind::S_GPROC32_ID);
----------------
Rather than having this assert, let's remove the return type from canonicalizeSymbolKind and do `SymbolKind NewKind = symbolKind(NewData);`. It's one load.


================
Comment at: lld/COFF/PDB.cpp:490-492
+    assert(NewKind != SymbolKind::S_GPROC32_ID);
+    assert(NewKind != SymbolKind::S_LPROC32_ID);
+    assert(NewKind != SymbolKind::S_PROC_ID_END);
----------------
These != assertions are trivial, IMO. They also don't represent internal errors, we emitted those records for a while, and cvdump still accepted our PDBs.


================
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) {
----------------
rnk wrote:
> This thing isn't really canonicalizing the symbol kind anymore, it's rewriting the symbol record. Maybe name it something like `rewriteSymbolForPDB`?
I still like `rewriteSymbolForPDB`


================
Comment at: lld/COFF/PDB.cpp:361
+    if (!TI->isSimple() && !TI->isNoneType()) {
+      ArrayRef<uint8_t> FuncIdData = IDTable.records()[TI->toArrayIndex()];
+      FuncIdRecord FID;
----------------
zturner wrote:
> rnk wrote:
> > We need an error check for OOB IPI index
> That should have already happened when we merged the type streams.  Wouldn't we have discarded any records that have invalid type index references, rather than emitting them with bad indices?
Yeah, that makes sense. Any invalid item index would've become the simple "not translated" type index.


================
Comment at: lld/COFF/PDB.cpp:488
+    remapTypesInSymbolRecord(File, Contents, IndexMap, IDTable, TypeRefs);
+    SymbolKind NewKind = canonicalizeSymbolKind(NewData, IDTable);
+    assert(NewKind == symbolKind(NewData));
----------------
zturner wrote:
> rnk wrote:
> > Let's keep this as part of the copySymbolForPdb.
> I actually think it should be here.  I tried moving it into `copySymbolForPdb` and the code becomes really ugly.  You end up having to duplicate a lot of the logic from `remapTypesInSymbolRecord` but forcing it to remap into the TPI stream's index space.  By doing it this way, each step is logically independent.  copy symbol literally just copies some memory like the comment says.  remap types does exactly what it says, and so does canonicalize.  I can call it remapSymbolKind or something like that if you prefer, but I kind of like the idea of keeping it as a sequence of 3 independent steps like this.
Right, I agree, it should happen after remapping. Let's call it something else, though. We're doing a lot more than canonicalizing the symbol kind. Ultimately, we're going to need logic here for filtering out `S_UDT` records.


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


https://reviews.llvm.org/D36426