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

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: lld/wasm/Config.h:58
   llvm::CachePruningPolicy ThinLTOCachePolicy;
+  llvm::Optional<std::vector<std::string>> Features;
 
----------------
ruiu wrote:
> 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.
This would not quite work. Currently an empty vector means "the output uses no features", which is different from an empty Optional which means "read the set of used features from the input objects."


================
Comment at: lld/wasm/Writer.cpp:873
 
 void Writer::calculateTargetFeatures() {
+  SmallSet<std::string, 8> Used;
----------------
ruiu wrote:
> 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?
I think this is reasonable as is. Splitting this into separate functions for computing TargetFeatures and for validating the TargetFeatures would require another loop over all the individual objects' target features. I also view the validation as an important part of computing the TargetFeatures set.


================
Comment at: lld/wasm/Writer.cpp:882
+  if (!InferFeatures) {
+    auto Features = Config->Features.getValue();
+    TargetFeatures.insert(Features.begin(), Features.end());
----------------
ruiu wrote:
> We don't use `auto` unless its type is obvious from the right-hand side (e.g. cast).
I switched to using a loop. Hopefully `auto` is ok in that context?


================
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
----------------
ruiu wrote:
>   TargetFeatures = Config->Features;
This does not work. `TargetFeatures` is a set of strings, but `Config->Features` is a vector of `char *`.


================
Comment at: lld/wasm/Writer.cpp:884-886
+    // No need to read or check features
+    if (!Config->CheckFeatures)
+      return;
----------------
ruiu wrote:
> Why don't you need to check features?
If the user chooses to explicitly supply features on the command line (using --features=...) and also elects not to validate the used features (using --no-check-features), then there is no reason to do more work here.


================
Comment at: lld/wasm/Writer.cpp:901
       case WASM_FEATURE_PREFIX_DISALLOWED:
         Disallowed.insert(Feature.Name);
         break;
----------------
ruiu wrote:
> 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.
Not quite. If --feature is given, we still compute the Used set from the input objects, then we check below that no features are Used that were not supplied on the command line by the user. If that check passes then we go on to validate against the Required and Disallowed sets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59274