[PATCH] D21768: Support CFI for WebAssembly target

[Peter Collingbourne via llvm-commits]

pcc added a comment.

Thanks for that writeup, it helped me understand what's going on here more clearly. Please also include those details in your commit message.

> Also, how do you represent null function pointers if indexes start at zero?


I'm still a little unclear about this. Suppose I have some code that looks like this:

  void f() {}
  
  bool foo(void (*fp)()) {
    return fp != nullptr;
  }

What result will `foo(f)` yield if this pass happens to assign index 0 to the function `f`, and why?

> Currently, calling dlsym finds the imported function, copies it to the function and table sections of the host, and returns its index for use in an indirect call. Clearly, this doesn't work well with CFI; different implementations include requiring a forward declaration of dynamically-linked functions for placeholder entries, fixing table offsets and set boundaries at runtime, etc. Alternatively, if tables with homogeneous type are used for CFI, then the tables from two modules could just be joined directly: two tables with the same type are merged, otherwise new tables are created and table references are updated.


Okay, I'm happy for the design here to match the current wasm design, and for it to evolve as wasm does.


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:191
@@ +190,3 @@
+
+    getTargetStreamer()->emitIndIdx(AsmPrinter::lowerConstant(cast<ConstantAsMetadata>(Idx->getOperand(0))->getValue()));
+  }
----------------
clang-format this line.

================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:684-685
@@ -685,4 +683,4 @@
 
 /// Given a disjoint set of type identifiers and functions, build a jump table
 /// for the functions, build the bit sets and lower the llvm.type.test calls.
 void LowerTypeTests::buildBitSetsFromFunctions(ArrayRef<Metadata *> TypeIds,
----------------
This comment belongs on `buildBitSetsFromFunctionsX86`.

================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:688-694
@@ -689,1 +687,9 @@
                                                ArrayRef<Function *> Functions) {
+  if (Arch == Triple::x86 || Arch == Triple::x86_64) {
+    buildBitSetsFromFunctionsX86(TypeIds, Functions);
+  } else if (Arch == Triple::wasm32 || Arch == Triple::wasm64) {
+    buildBitSetsFromFunctionsWASM(TypeIds, Functions);
+  } else {
+    report_fatal_error("Unsupported architecture for jump tables");
+  }
+}
----------------
Remove all braces here.

================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:840
@@ +839,3 @@
+  for (Function *F : Functions)
+    GlobalLayout[F] = IndirectIndex++;
+
----------------
You could set the function metadata here instead of in the loop below.

================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:842
@@ +841,3 @@
+
+  // Pass a NULL pointer as the subtracted "jump table" offset.
+  lowerTypeTestCalls(TypeIds,
----------------
Please explain *why* you are passing a null pointer here (i.e. because wasm does not use jump tables directly...)

================
Comment at: test/Transforms/LowerTypeTests/function-disjoint.ll:3
@@ +2,3 @@
+; RUN: opt -S -lowertypetests -mtriple=wasm32-unknown-unknown < %s | FileCheck --check-prefix=WASM32 %s
+
+; Tests that we correctly handle bitsets with disjoint call target sets.
----------------
I think this should be fine because we aren't using the backend here.


http://reviews.llvm.org/D21768