[lld][ELF] Add negative test coverage for unknown "-z" flags.

[kristina]

Summary

This makes the code cleaner and adds some coverage for invalid flags being passed as -z <unknown flag>. Almost an NFC change but just wanted to confirm.

[kristina]

Revised, newline at the end of the test file.

[kristina]

No regressions in ELF test suite and test runs just fine:

=> ninja check-lld-elf

[0/1] Running lit suite /q/src/llvm-taint-8.0/tools/lld/test/ELF
Testing Time: 9.32s
  Expected Passes    : 1054
  Unsupported Tests  : 332

[ruiu]

Thank you for the patch. But honestly I wouldn't do this personally because the former code only depends on the usual equality check while the new code uses SmallDenseSet which is undeniably (slightly) more complicated than the comparison. From the perspective of code readability, I don't think there's too much difference, it's perhaps a matter of taste. So I think I prefer code that is simple, which is perhaps too simple that it sometimes even looks a bit stupid.

[kristina]

Shall I fully abandon the revision or want me to leave the test for unknown flag failure in? (Unless one already exists and I missed it, additional coverage can't hurt regardless)

[kristina]

Revised to drop the change.

[ruiu]

We do have a test for an unknown -z value. See test/ELF/driver.test.

[kristina]

Ah alright no problem, seems I didn't notice. Thanks for the quick review.

[joerg]

Well, I do like the general idea. I would go with a sorted static array and a binary search though. This is not really time critical, but the array version would be smaller in terms of code without involving run-time allocations.

ELF/Driver.cpp

if (K1 == Arg->getValue())
       return true;
     if (K2 == Arg->getValue())
       return false;
   }
   return Default;
 }
 
 static bool isKnownZFlag(StringRef S) {
-  return S == "combreloc" || S == "copyreloc" || S == "defs" ||
-         S == "execstack" || S == "global" || S == "hazardplt" ||
-         S == "initfirst" || S == "interpose" ||
-         S == "keep-text-section-prefix" || S == "lazy" || S == "muldefs" ||
-         S == "nocombreloc" || S == "nocopyreloc" || S == "nodelete" ||
-         S == "nodlopen" || S == "noexecstack" ||
-         S == "nokeep-text-section-prefix" || S == "norelro" || S == "notext" ||
-         S == "now" || S == "origin" || S == "relro" || S == "retpolineplt" ||
-         S == "rodynamic" || S == "text" || S == "wxneeded" ||
-         S.startswith("max-page-size=") || S.startswith("stack-size=");
+  static llvm::SmallDenseSet<StringRef> KnownFlags {
+    "combreloc", "copyreloc", "defs",
+    "execstack", "global", "hazardplt",
+    "initfirst", "interpose", "keep-text-section-prefix",
+    "lazy", "muldefs", "nocombreloc",
+    "nocopyreloc", "nodelete", "nodlopen", 
+    "noexecstack", "nokeep-text-section-prefix", "norelro", 
+    "notext", "now", "origin",
+    "relro", "retpolineplt", "rodynamic",
+    "text", "wxneeded"
+  };
+   
+  return KnownFlags.count(S) || S.startswith("max-page-size=") ||
+         S.startswith("stack-size=");
 }
 
 // Report an error for an unknown -z option.
 static void checkZOptions(opt::InputArgList &Args) {
   for (auto *Arg : Args.filtered(OPT_z))
     if (!isKnownZFlag(Arg->getValue()))
       error("unknown -z value: " + StringRef(Arg->getValue()));
 }

test/ELF/invalid/zunknown-flag.test

+# RUN: not ld.lld -z obviouslyunknownflag 2>&1 | FileCheck --check-prefix=ERR %s
+# ERR: error: unknown -z value: obviouslyunknownflag
+