[PATCH] D59274: [WebAssembly] Add linker options to control feature checking

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/wasm/Config.h:58
   llvm::CachePruningPolicy ThinLTOCachePolicy;
+  llvm::Optional<std::vector<std::string>> Features;
 
----------------
It seems like you can just use `std::vector<std::string>` instead of `Optional<std::vector<std::string>>`. If there's nothing, leave the vector empty.


================
Comment at: lld/wasm/Driver.cpp:345-352
+  using FeatureList = Optional<std::vector<std::string>>;
+  if (auto FeatureArg = Args.getLastArg(OPT_features)) {
+    auto &Values = FeatureArg->getValues();
+    Config->Features =
+        FeatureList(std::vector<std::string>(Values.begin(), Values.end()));
+  } else {
+    Config->Features = FeatureList();
----------------
This use of `using` seems unusual in the lld codebase. A more common way of writing it  in lld is:

  if (auto *Arg = Args.getLastArg(OPT_features))
    for (StringRef S : Args.getLastArg(OPT_features))
      Config->Features.push_back(S);


================
Comment at: lld/wasm/Options.td:163
+
+def features: C<"features=">,
+  HelpText<"Comma-separated used features, inferred from input objects by default.">;
----------------
Unless you have a plan to use `C` multiple times, I wouldn't define `C`. You can do that when you have more than one occurrence of the same pattern.


================
Comment at: lld/wasm/Writer.cpp:873
 
 void Writer::calculateTargetFeatures() {
+  SmallSet<std::string, 8> Used;
----------------
This function sounds like it computes TargetFeatures, but it does more than that -- it tries to find a conflict of feature sets. Is this a good name or a design?


================
Comment at: lld/wasm/Writer.cpp:882
+  if (!InferFeatures) {
+    auto Features = Config->Features.getValue();
+    TargetFeatures.insert(Features.begin(), Features.end());
----------------
We don't use `auto` unless its type is obvious from the right-hand side (e.g. cast).


================
Comment at: lld/wasm/Writer.cpp:883
+    auto Features = Config->Features.getValue();
+    TargetFeatures.insert(Features.begin(), Features.end());
+    // No need to read or check features
----------------
  TargetFeatures = Config->Features;


================
Comment at: lld/wasm/Writer.cpp:884-886
+    // No need to read or check features
+    if (!Config->CheckFeatures)
+      return;
----------------
Why don't you need to check features?


================
Comment at: lld/wasm/Writer.cpp:901
       case WASM_FEATURE_PREFIX_DISALLOWED:
         Disallowed.insert(Feature.Name);
         break;
----------------
So, if --feature is given, you ignore Used but use Required and Disallowed? It seems a bit inconsistent to me. I'm not sure if it's correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59274