[PATCH] D34168: [cfi] CFI-ICall for ThinLTO.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:38
 #include <vector>
+#include <set>
 
----------------
Sort by name.


================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:269
+
+    if (io.outputting()) {
+      StringSetYaml CfiFunctionDefs(index.CfiFunctionDefs);
----------------
eugenis wrote:
> An alternative to this is rewriting SequenceTraits in terms of begin/end iterators.
> The current interface does not work for std::set at all because it is index-based.
Yes, that's one possibility. But for now I'd suggest performing the vector/set conversions directly here, then relying on the built-in handling of `std::vector` which would avoid the need for the `StringSetYaml` class.
```
if (io.outputting()) {
  std::vector<std::string> CfiFunctionDefs(index.CfiFunctionDefs.begin(), index.CfiFunctionDefs.end());
  io.mapOptional("CfiFunctionDefs", CfiFunctionDefs);

  // same for CfiFunctionDecls
} else {
  std::vector<std::string> CfiFunctionDefs;
  io.mapOptional("CfiFunctionDefs", CfiFunctionDefs);
  index.CfiFunctionDefs = {CfiFunctionDefs.begin(), CfiFunctionDefs.end()};

  // same for CfiFunctionDecls
}
```


================
Comment at: include/llvm/Support/YAMLTraits.h:662
+    if (this->canElideEmptySequence() &&
+        SequenceTraits<T>::size(*this, Val) == 0)
       return;
----------------
This is only necessary because of `StringSetYaml`, right?


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5254
     }
+    case bitc::FS_CFI_FUNCTIONS: {
+      std::set<std::string> &CfiFunctionDefs = TheIndex.cfiFunctionDefs();
----------------
I'd do this with two record types, one for defs and the other for decls, but up to you.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3853
+    const std::map<std::string, GVSummaryMapTy> *ModuleToSummariesForIndex) {
+  IndexBitcodeWriter IndexWriter(*Stream, StrtabBuilder, *Index, ModuleToSummariesForIndex);
+  IndexWriter.write();
----------------
clang-format


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:1444
+    for (auto &F : M) {
+      if (ImportSummary->cfiFunctionDefs().count(F.getName()))
+        Defs.push_back(&F);
----------------
Will this do the right thing if a (non-address-taken) function with local linkage has the same name as an unrelated function in another module with external linkage? I think you need to check that the function has external linkage before testing for set membership.


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:1481
+
+  DenseMap<StringRef, std::pair<bool, MDNode *>> ExportedFunctions;
+  if (ExportSummary) {
----------------
I would declare a struct inline here instead of using a pair, to make this a little more self-documenting.


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:1498
+        auto &V = ExportedFunctions[FunctionName];
+        if (isDefinition >= V.first)
+          V = {isDefinition, FuncMD};
----------------
I think this would be clearer as `if (!V.first)`.

Could `V.first` be uninitialized at this point?


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:1504
+        StringRef FunctionName = P.first;
+        // bool isDefinition = P.second.first;
+        MDNode *FuncMD = P.second.second;
----------------
Remove line.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:35
 // changing visibility and appending the given ModuleId.
 void promoteInternals(Module &ExportM, Module &ImportM, StringRef ModuleId) {
+  NamedMDNode *CfiFunctionsMD = ImportM.getNamedMetadata("cfi.functions");
----------------
Rebuilding the metadata in here doesn't seem very elegant. Maybe instead you could:
1. compute a `SetVector<GlobalValue *>` of which functions require jump table entries
2. pass that set into this function and use it to control whether to promote
3. once it returns, produce the metadata in the merged module by enumerating the set.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:336
+      unsigned Linkage =
+          F.isDeclarationForLinker() ? (F.isWeakForLinker() ? 2 : 1) : 0;
+      Elts.push_back(ConstantAsMetadata::get(
----------------
Can we declare an enum somewhere instead of using these magic numbers?

I would also rewrite with if/else.


Repository:
  rL LLVM

https://reviews.llvm.org/D34168