[PATCH] D45213: [COFF][LLD] Add link support for precompiled headers .objs

[Alexandre Ganea via Phabricator via llvm-commits]

aganea marked 9 inline comments as done.
aganea added inline comments.


================
Comment at: lld/COFF/PDB.cpp:293
   // merge any type information.
-  if (Optional<TypeServer2Record> TS = maybeReadTypeServerRecord(Types))
-    return maybeMergeTypeServerPDB(File, *TS);
+  if (Optional<TypeServer2Record> TS =
+                      maybeReadRecord<TypeServer2Record>(Types, LF_TYPESERVER2))
----------------
zturner wrote:
> Would it be possible to write this as something like:
> 
> ```
> if (isExternalTypeStreamRecord(Types))
>   return handleExternalTypeStreamRecord(Types);
> ```
> 
> Then this new handle function could handle both `LF_TYPESERVER2` and `LF_PRECOMP`, and we wouldn't need the weird function with the boolean anymore
Unfortunately we can't. In one case (PDB), we must return the full TPI/IPI map; in the other case (PCH) we must load the TPI map and fallthrough.


================
Comment at: lld/COFF/PDB.cpp:542-544
+  static CVIndexMap emptyMap;
+  emptyMap.IsPrecompiledTypeMap = true;
+  return emptyMap;
----------------
zturner wrote:
> Eww.  How about returning an `Expected<const CVIndexMap&>` instead?
Please unsee this.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp:489-492
+  // For .obj files containing precompiled types, we need to extract the
+  // signature, through EndPrecompRecord. This is done here for performance
+  // reasons, to avoid re-parsing the Types stream; in most cases, this .obj
+  // will not be a precompiled types file.
----------------
zturner wrote:
> I noticed that when precompiled headers are used, the `S_OBJNAME` symbol also has a matching signature.  Can / should this be used somehow?
Yes, and the new revision (https://reviews.llvm.org/D45213?id=154301) is using it. It was probably meant there at the begining of the stream to bail out quickly, if the associated signature doesn't match the PCH session.


https://reviews.llvm.org/D45213