Skip to content

[InstallAPI] add JSON option to pass X<label> arguments #91770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 21, 2024

Conversation

cyndyishida
Copy link
Member

No description provided.

@cyndyishida cyndyishida requested review from zixu-w and ributzka May 10, 2024 17:20
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 10, 2024
@llvmbot
Copy link
Member

llvmbot commented May 10, 2024

@llvm/pr-subscribers-clang

Author: Cyndy Ishida (cyndyishida)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/91770.diff

9 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticInstallAPIKinds.td (+1-1)
  • (modified) clang/test/InstallAPI/alias_list.test (+1-1)
  • (modified) clang/test/InstallAPI/exclusive-passes-2.test (+9)
  • (added) clang/test/InstallAPI/exclusive-passes-3.test (+86)
  • (modified) clang/test/InstallAPI/exclusive-passes.test (+15)
  • (modified) clang/test/InstallAPI/invalid-exclusive-passes.test (+33)
  • (modified) clang/tools/clang-installapi/InstallAPIOpts.td (+3)
  • (modified) clang/tools/clang-installapi/Options.cpp (+72-2)
  • (modified) clang/tools/clang-installapi/Options.h (+2)
diff --git a/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td b/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td
index 674742431dcb2..944b2a38b6e96 100644
--- a/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td
+++ b/clang/include/clang/Basic/DiagnosticInstallAPIKinds.td
@@ -24,7 +24,7 @@ def err_no_matching_target : Error<"no matching target found for target variant
 def err_unsupported_vendor : Error<"vendor '%0' is not supported: '%1'">;
 def err_unsupported_environment : Error<"environment '%0' is not supported: '%1'">;
 def err_unsupported_os : Error<"os '%0' is not supported: '%1'">;
-def err_cannot_read_input_list : Error<"could not read %select{alias list|filelist}0 '%1': %2">;
+def err_cannot_read_input_list : Error<"could not read %0 input list '%1': %2">;
 def err_invalid_label: Error<"label '%0' is reserved: use a different label name for -X<label>">;
 } // end of command line category.
 
diff --git a/clang/test/InstallAPI/alias_list.test b/clang/test/InstallAPI/alias_list.test
index 3e12221e088c4..aba7e395cca95 100644
--- a/clang/test/InstallAPI/alias_list.test
+++ b/clang/test/InstallAPI/alias_list.test
@@ -23,7 +23,7 @@
 ; RUN: -o %t/AliasList.tbd 2>&1 | FileCheck -allow-empty %s  \
 ; RUN: --check-prefix=INVALID
 
-; INVALID: error: could not read alias list {{.*}} missing alias for: _hidden
+; INVALID: error: could not read symbol alias input list {{.*}}invalid.txt': invalid input format: missing alias for: _hidden
 
 ;--- Frameworks/AliasList.framework/Headers/AliasList.h
 // simple alias from one symbol to another.
diff --git a/clang/test/InstallAPI/exclusive-passes-2.test b/clang/test/InstallAPI/exclusive-passes-2.test
index 3e7a6d777d5af..132b27df383c4 100644
--- a/clang/test/InstallAPI/exclusive-passes-2.test
+++ b/clang/test/InstallAPI/exclusive-passes-2.test
@@ -11,6 +11,15 @@
 ; RUN: -DFoo -XApple -DDarwin=1 -XElf -DNONDarwin=1 2>&1 | FileCheck -allow-empty %s 
 ; RUN: llvm-readtapi --compare %t/output.tbd %t/expected.tbd 2>&1 | FileCheck -allow-empty %s
 
+; RUN: clang-installapi -target arm64-apple-macos12 \
+; RUN: -install_name @rpath/libfoo.dylib \
+; RUN: -current_version 1 -compatibility_version 1 \
+; RUN: -I%S/Inputs/LibFoo/usr/include -dynamiclib \
+; RUN: -extra-public-header %S/Inputs/LibFoo/usr/include/foo.h \
+; RUN: -o %t/output2.tbd \
+; RUN: -DFoo -optionlist %t/options.json 2>&1 | FileCheck -allow-empty %s 
+; RUN: llvm-readtapi --compare %t/output.tbd %t/expected.tbd 2>&1 | FileCheck -allow-empty %s
+
 ; CHECK-NOT: error
 ; CHECK-NOT: warning
 
diff --git a/clang/test/InstallAPI/exclusive-passes-3.test b/clang/test/InstallAPI/exclusive-passes-3.test
new file mode 100644
index 0000000000000..3a9b64c9f7b86
--- /dev/null
+++ b/clang/test/InstallAPI/exclusive-passes-3.test
@@ -0,0 +1,86 @@
+; RUN: rm -rf %t
+; RUN: split-file %s %t
+
+// "Apple" label has split options between the optionlist & command line. 
+; RUN: clang-installapi -target arm64-apple-macos12 \
+; RUN: -install_name @rpath/libfoo.dylib -current_version 1 \
+; RUN: -compatibility_version 1 \
+; RUN: -extra-public-header %t/usr/include/opts.h \
+; RUN: -optionlist %t/options.json -XApple -DCLI_OPT=1 \
+; RUN: -I%S/Inputs/LibFoo/usr/include \
+; RUN: -I%t/usr/include -dynamiclib -o %t/output.tbd 2>&1 | FileCheck %s -allow-empty 
+; RUN: llvm-readtapi --compare %t/output.tbd %t/expected.tbd 2>&1 | FileCheck -allow-empty %s
+
+// Validate duplicated options give same result.
+; RUN: clang-installapi -target arm64-apple-macos12 \
+; RUN: -install_name @rpath/libfoo.dylib -current_version 1 \
+; RUN: -compatibility_version 1 \
+; RUN: -extra-public-header %t/usr/include/opts.h \
+; RUN: -optionlist %t/options.json -XApple -DCLI_OPT=1 \
+; RUN: -I%S/Inputs/LibFoo/usr/include \
+; RUN: -XApple -DDarwin -XElf -DNONDarwin \
+; RUN: -I%t/usr/include -dynamiclib -o %t/output2.tbd 2>&1 | FileCheck %s -allow-empty 
+; RUN: llvm-readtapi --compare %t/output2.tbd %t/expected.tbd 2>&1 | FileCheck -allow-empty %s
+
+; CHECK-NOT: error
+; CHECK-NOT: warning
+
+;--- usr/include/opts.h
+#ifndef OPTS_H
+#define OPTS_H
+#include <macro_defs.h>
+
+#if defined(CLI_OPT) && CLI_OPT 
+  #define SUFFIX "$final"
+#else 
+  #define SUFFIX 
+#endif 
+
+
+#define __STRING(x)     #x
+#define PLATFORM_ALIAS(sym)	__asm("_" __STRING(sym) DARWIN LINUX SUFFIX)
+extern int foo() PLATFORM_ALIAS(foo);
+
+#endif 
+
+;--- expected.tbd
+{
+  "main_library": {
+    "exported_symbols": [
+      {
+        "text": {
+          "global": [
+            "_foo$darwin$final",
+            "_foo$linux",
+            "_foo"
+          ]
+        }
+      }
+    ],
+    "flags": [
+      {
+        "attributes": [
+          "not_app_extension_safe"
+        ]
+      }
+    ],
+    "install_names": [
+      {
+        "name": "@rpath/libfoo.dylib"
+      }
+    ],
+    "target_info": [
+      {
+        "min_deployment": "12",
+        "target": "arm64-macos"
+      }
+    ]
+  },
+  "tapi_tbd_version": 5
+}
+
+//--- options.json
+{
+  "Apple" : ["-DDarwin=1"],
+  "Elf" : ["-DNONDarwin=1"]
+}
diff --git a/clang/test/InstallAPI/exclusive-passes.test b/clang/test/InstallAPI/exclusive-passes.test
index 29b0fc3d7a2aa..8e2d01ebaab19 100644
--- a/clang/test/InstallAPI/exclusive-passes.test
+++ b/clang/test/InstallAPI/exclusive-passes.test
@@ -10,6 +10,15 @@
 ; RUN: -o %t/output.tbd -v 2>&1 | FileCheck %s --check-prefix=INSTALLAPI
 ; RUN: llvm-readtapi --compare %t/output.tbd %t/expected.tbd 2>&1 | FileCheck -allow-empty %s
 
+// Try with -optionlist.
+; RUN: clang-installapi \
+; RUN: -target arm64-apple-macos12 -install_name @rpath/libfoo.dylib \
+; RUN: -current_version 1 -compatibility_version 1 \
+; RUN: -I%S/Inputs/LibFoo/usr/include -dynamiclib \
+; RUN: -extra-public-header %S/Inputs/LibFoo/usr/include/public.h \
+; RUN: -optionlist %t/options.json -o %t/output2.tbd 2>&1 | FileCheck %s -allow-empty 
+; RUN: llvm-readtapi --compare %t/output2.tbd %t/expected.tbd 2>&1 | FileCheck -allow-empty %s
+
 ; CHECK-NOT: error
 ; CHECK-NOT: warning
 
@@ -17,6 +26,12 @@
 ; INSTALLAPI: Apple Public Headers:
 ; INSTALLAPI: Elf Public Headers:
 
+;--- options.json
+{
+  "Apple" : ["-DDarwin=1"],
+  "Elf" : ["-DNONDarwin=1"]
+}
+
 ;--- expected.tbd
 {
   "main_library": {
diff --git a/clang/test/InstallAPI/invalid-exclusive-passes.test b/clang/test/InstallAPI/invalid-exclusive-passes.test
index c23c918f0bfbb..24b3e3193866c 100644
--- a/clang/test/InstallAPI/invalid-exclusive-passes.test
+++ b/clang/test/InstallAPI/invalid-exclusive-passes.test
@@ -30,6 +30,39 @@
 ; RUN: -o %t/output.tbd 2>&1 | FileCheck %s --check-prefix=INVALID_PROJECT_OPT
 ; INVALID_PROJECT_OPT: error: invalid argument '-Xproject' not allowed with '-fprofile-instr-generate'
 
+// Validate arguments not allowed with -X passed via json
+; RUN: not clang-installapi -target arm64-apple-macos12 \
+; RUN: -install_name @rpath/libfoo.dylib -current_version 1 -compatibility_version 1 \
+; RUN: -optionlist %t/options.json -I/fake/path \
+; RUN: -I%t -dynamiclib -o %t/output.tbd 2>&1 | FileCheck %s --check-prefix=INVALID_JSON_OPT
+; INVALID_JSON_OPT: error: invalid argument '-XApple' not allowed with '-I/fake/path'
+
+// Validate invalid json path
+; RUN: not clang-installapi -target arm64-apple-macos12 \
+; RUN: -install_name @rpath/libfoo.dylib -current_version 1 \
+; RUN: -compatibility_version 1 -optionlist %t/invalid_loc.json \
+; RUN: -I/fake/path -I%t -dynamiclib \
+; RUN: -o %t/output.tbd %t 2>&1 | FileCheck %s --check-prefix=INVALID_JSON_LOC
+; INVALID_JSON_LOC: error: cannot open file {{.*}}invalid_loc.json': No such file or directory
+
+// Validate invalid json format 
+; RUN: not clang-installapi -target arm64-apple-macos12 \
+; RUN: -install_name @rpath/libfoo.dylib -current_version 1 \
+; RUN: -compatibility_version 1 -optionlist %t/invalid_format.json \
+; RUN: -I/fake/path -isysroot %sysroot -I%t -dynamiclib \
+; RUN: -o %t/output.tbd %t 2>&1 | FileCheck %s --check-prefix=INVALID_JSON_FORMAT
+; INVALID_JSON_FORMAT: error: could not read option input list {{.*}}invalid_format.json': invalid input format
+
+;--- options.json
+{
+  "Apple" : ["-I/fake/path"]
+}
+
+;--- invalid_format.json
+{
+  "Apple" : {"opt"  : "-I/fake/path"}
+}
+
 ;--- inputs.json
 {
   "headers": [ ],
diff --git a/clang/tools/clang-installapi/InstallAPIOpts.td b/clang/tools/clang-installapi/InstallAPIOpts.td
index a95a7a80a9d20..fc0fbe929c887 100644
--- a/clang/tools/clang-installapi/InstallAPIOpts.td
+++ b/clang/tools/clang-installapi/InstallAPIOpts.td
@@ -99,6 +99,9 @@ def X__ : Joined<["-"], "X">,
   HelpText<"Pass <arg> to run unique clang invocation identified as <label>">, 
   MetaVarName<"<label> <arg>">;
 
+def option_list : Separate<["-"], "optionlist">, MetaVarName<"<path>">, 
+  HelpText<"Specifies the <path> to a file that contains X<label> arguments to parse.">;
+
 //
 /// Overidden clang options for different behavior.
 //
diff --git a/clang/tools/clang-installapi/Options.cpp b/clang/tools/clang-installapi/Options.cpp
index 5396ad23620b9..7ba71bb3c9602 100644
--- a/clang/tools/clang-installapi/Options.cpp
+++ b/clang/tools/clang-installapi/Options.cpp
@@ -13,6 +13,7 @@
 #include "clang/InstallAPI/HeaderFile.h"
 #include "clang/InstallAPI/InstallAPIDiagnostic.h"
 #include "llvm/BinaryFormat/Magic.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/Program.h"
 #include "llvm/TargetParser/Host.h"
 #include "llvm/TextAPI/DylibReader.h"
@@ -82,6 +83,47 @@ static llvm::opt::OptTable *createDriverOptTable() {
   return new DriverOptTable();
 }
 
+/// Parse JSON input into argument list.
+///
+/* Expected input format.
+ *  { "label" : ["-ClangArg1", "-ClangArg2"] }
+ */
+///
+/// Input is interpreted as "-Xlabel ClangArg1 -XLabel ClangArg2".
+static Expected<llvm::opt::InputArgList>
+getArgListFromJSON(const StringRef Input, llvm::opt::OptTable *Table,
+                   std::vector<std::string> &Storage) {
+  using namespace json;
+  Expected<Value> ValOrErr = json::parse(Input);
+  if (!ValOrErr)
+    return ValOrErr.takeError();
+
+  const Object *Root = ValOrErr->getAsObject();
+  if (!Root)
+    return llvm::opt::InputArgList();
+
+  for (const auto &KV : *Root) {
+    const Array *ArgList = KV.getSecond().getAsArray();
+    std::string Label = "-X" + KV.getFirst().str();
+    if (!ArgList)
+      return make_error<TextAPIError>(TextAPIErrorCode::InvalidInputFormat);
+    for (auto Arg : *ArgList) {
+      std::optional<StringRef> ArgStr = Arg.getAsString();
+      if (!ArgStr)
+        return make_error<TextAPIError>(TextAPIErrorCode::InvalidInputFormat);
+      Storage.emplace_back(Label);
+      Storage.emplace_back(*ArgStr);
+    }
+  }
+
+  std::vector<const char *> CArgs(Storage.size());
+  llvm::for_each(Storage,
+                 [&CArgs](StringRef Str) { CArgs.emplace_back(Str.data()); });
+
+  unsigned MissingArgIndex, MissingArgCount;
+  return Table->ParseArgs(CArgs, MissingArgIndex, MissingArgCount);
+}
+
 bool Options::processDriverOptions(InputArgList &Args) {
   // Handle inputs.
   llvm::append_range(DriverOpts.FileLists,
@@ -345,6 +387,31 @@ bool Options::processXarchOption(InputArgList &Args, arg_iterator Curr) {
   return true;
 }
 
+bool Options::processOptionList(InputArgList &Args,
+                                llvm::opt::OptTable *Table) {
+  Arg *A = Args.getLastArg(OPT_option_list);
+  if (!A)
+    return true;
+
+  const StringRef Path = A->getValue(0);
+  auto InputOrErr = FM->getBufferForFile(Path);
+  if (auto Err = InputOrErr.getError()) {
+    Diags->Report(diag::err_cannot_open_file) << Path << Err.message();
+    return false;
+  }
+  // Backing storage referenced for argument processing.
+  std::vector<std::string> Storage;
+  auto ArgsOrErr =
+      getArgListFromJSON((*InputOrErr)->getBuffer(), Table, Storage);
+
+  if (auto Err = ArgsOrErr.takeError()) {
+    Diags->Report(diag::err_cannot_read_input_list)
+        << "option" << Path << toString(std::move(Err));
+    return false;
+  }
+  return processInstallAPIXOptions(*ArgsOrErr);
+}
+
 bool Options::processLinkerOptions(InputArgList &Args) {
   // Handle required arguments.
   if (const Arg *A = Args.getLastArg(drv::OPT_install__name))
@@ -507,6 +574,9 @@ Options::processAndFilterOutInstallAPIOptions(ArrayRef<const char *> Args) {
   if (!processInstallAPIXOptions(ParsedArgs))
     return {};
 
+  if (!processOptionList(ParsedArgs, Table.get()))
+    return {};
+
   DriverOpts.Demangle = ParsedArgs.hasArg(OPT_demangle);
 
   if (auto *A = ParsedArgs.getLastArg(OPT_filetype)) {
@@ -815,7 +885,7 @@ InstallAPIContext Options::createContext() {
     Expected<AliasMap> Result = parseAliasList(Buffer.get());
     if (!Result) {
       Diags->Report(diag::err_cannot_read_input_list)
-          << /*IsFileList=*/false << ListPath << toString(Result.takeError());
+          << "symbol alias" << ListPath << toString(Result.takeError());
       return Ctx;
     }
     Aliases.insert(Result.get().begin(), Result.get().end());
@@ -836,7 +906,7 @@ InstallAPIContext Options::createContext() {
     if (auto Err = FileListReader::loadHeaders(std::move(Buffer.get()),
                                                Ctx.InputHeaders, FM)) {
       Diags->Report(diag::err_cannot_read_input_list)
-          << /*IsFileList=*/true << ListPath << std::move(Err);
+          << "header file" << ListPath << std::move(Err);
       return Ctx;
     }
   }
diff --git a/clang/tools/clang-installapi/Options.h b/clang/tools/clang-installapi/Options.h
index fd1e10065d102..b37f91efbda72 100644
--- a/clang/tools/clang-installapi/Options.h
+++ b/clang/tools/clang-installapi/Options.h
@@ -161,6 +161,8 @@ class Options {
   bool processXarchOption(llvm::opt::InputArgList &Args, arg_iterator Curr);
   bool processXplatformOption(llvm::opt::InputArgList &Args, arg_iterator Curr);
   bool processXprojectOption(llvm::opt::InputArgList &Args, arg_iterator Curr);
+  bool processOptionList(llvm::opt::InputArgList &Args,
+                         llvm::opt::OptTable *Table);
 
 public:
   /// The various options grouped together.

@cyndyishida cyndyishida merged commit c9dc52d into llvm:main May 21, 2024
3 of 4 checks passed
@cyndyishida cyndyishida deleted the eng/PR-JsonOption branch May 21, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants