[PATCH] D63978: Clang Interface Stubs merger plumbing for Driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:3372
+      if (Phase == phases::IfsMerge) {
+        assert(Phase == PL.back() && "merging must be final compilation step.");
+        MergerInputs.push_back(Current);
----------------
Does the interface merging have to be the last step?  I could see interface merging preceding linking just fine.


================
Comment at: clang/lib/Driver/Driver.cpp:3468
+  case phases::IfsMerge:
     llvm_unreachable("link action invalid here.");
   case phases::Preprocess: {
----------------
Please update the unreachable message


================
Comment at: clang/test/InterfaceStubs/driver-test.cpp:17
+// CHECK-DAG: _Z8weakFuncv
+
+int foo(int bar) {
----------------
Should we not ensure that no other symbols are exported?


================
Comment at: clang/test/InterfaceStubs/merge-conflict-test.cpp:3
+
+// -x c so that we dont have name-mangling.
+// RUN: not %clang -target x86_64-linux-gnu -x c -o libfoo.so -emit-interface-stubs \
----------------
Why not name the file `merge-conflict-test.c`?


================
Comment at: clang/test/InterfaceStubs/merge-conflict-test.cpp:4
+// -x c so that we dont have name-mangling.
+// RUN: not %clang -target x86_64-linux-gnu -x c -o libfoo.so -emit-interface-stubs \
+// RUN: %s %S/driver-test.cpp 2>&1 | FileCheck %s
----------------
Why not leave the `-target` off entirely?  That will allow you to drop the restriction of the `x86-registered-target`


================
Comment at: clang/test/InterfaceStubs/object-double.cpp:2
+// REQUIRES: x86-registered-target
+// RUN: not %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s %S/object.cpp 2>&1 | \
+// RUN: FileCheck %s
----------------
Similar


================
Comment at: clang/test/InterfaceStubs/object-float.cpp:2
+// REQUIRES: x86-registered-target
+// RUN: not %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s %S/object.cpp 2>&1 | \
+// RUN: FileCheck %s
----------------
And here


================
Comment at: clang/test/InterfaceStubs/object.cpp:5
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
-// RUN: -interface-stub-version=experimental-ifs-v1 %s | \
+// RUN: %clang -c -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s | \
 // RUN: FileCheck -check-prefix=CHECK-SYMBOLS %s
----------------
Might as well as do it here as well


================
Comment at: clang/test/InterfaceStubs/object.ifs:1
+# RUN: %clang -emit-merged-ifs -target x86_64-unknown-linux-gnu \
+# RUN: -emit-interface-stubs  -o - %s | \
----------------
Can we drop the target and get this to be portable?


================
Comment at: clang/test/InterfaceStubs/template-namespace-function.cpp:2
 // REQUIRES: x86-registered-target
-// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
-// RUN: -interface-stub-version=experimental-ifs-v1 %s | \
+// RUN: %clang -c -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s | \
 // RUN: FileCheck %s
----------------
Try to drop the x86 requirement throughout


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63978/new/

https://reviews.llvm.org/D63978