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

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

How big are the object files?



================
Comment at: lld/trunk/COFF/PDB.cpp:326-327
 
-static Optional<TypeServer2Record>
-maybeReadTypeServerRecord(CVTypeArray &Types) {
-  auto I = Types.begin();
-  if (I == Types.end())
-    return None;
-  const CVType &Type = *I;
-  if (Type.kind() != LF_TYPESERVER2)
-    return None;
-  TypeServer2Record TS;
-  if (auto EC = TypeDeserializer::deserializeAs(const_cast<CVType &>(Type), TS))
-    fatal("error reading type server record: " + toString(std::move(EC)));
-  return std::move(TS);
+// Microsoft objects which use precompiled headers always start their symbol
+// stream with a S_OBJNAME record. This record contains the signature/key of the
+// current PCH session. The signature must be same for all objects which depend
----------------
FWIW, all object files always start with an `S_OBJNAME`, even if there is no PCH support.  But if there is no PCH support, the signature field will just be 0.


================
Comment at: lld/trunk/COFF/PDB.cpp:355
+        return ObjName->Signature;
+    }
+  }
----------------
I think we should just `return 0;` here.  As you said, if it's there, it will be first, so if it's not first, there's no point to continue looking in subsequent `.debug$S` sections.


================
Comment at: lld/trunk/COFF/PDB.cpp:425-426
+    // with the precompiled headers object type stream.
+    Types.setUnderlyingStream(
+        Types.getUnderlyingStream().drop_front(FirstType->length()));
+  }
----------------
I almost wonder if we should add `CVTypeArray::drop_front()`.  You don't have to do it in this patch, just something I thought of.


================
Comment at: lld/trunk/COFF/PDB.cpp:626-627
+// Finds by name of full path an object provided on the command line
+Optional<std::pair<ObjFile *, std::string>>
+findObjByName(StringRef NameOrPath) {
+  SmallString<128> CurrentPath;
----------------
Can you add `static` to this function?


================
Comment at: lld/trunk/COFF/PDB.cpp:635
+    sys::fs::make_absolute(CurrentPath);
+    sys::path::native(CurrentPath, sys::path::Style::windows);
+
----------------
In another patch we were discussing whether we should stop using `path::Style::windows`.  When cross-compiling you might be on non-Windows, and these path comparisons would be incorrect in that case.  So I think we should probably just use the default native style here.


================
Comment at: lld/trunk/COFF/PDB.cpp:638-639
+    // First compare with the full path name
+    if (CurrentPath.equals_lower(NameOrPath))
+      return std::make_pair(F, CurrentPath.str().str());
+
----------------
It's hard to know exactly what to do here.  Is `equals_lower` correct or `equals`?  It seems like we need a `sys::path::equals_path()` that uses case-insensitive comparison on Windows and case-sensitive on Linux.  Can you add `equals_path()` as a global function in this file and use that here instead?


================
Comment at: lld/trunk/test/COFF/precomp-link.test:10
+
+# // precomp.h
+# #pragma once
----------------
The # signs are confusing.  I don't think you need to put any marker in front of them.  You can just write it as code.  lit and FileCheck ignore comment markers at the beginning of lines. and only match or run lines that have a RUN or CHECK line after any comments.  So just putting raw code directly in the file is fine.


https://reviews.llvm.org/D45213