Intuitively handle space in joined option syntax (#3024)

* Intuitively handle space in joined option syntax

A frequent mistake is to pass arguments to the compiler API where the
option is in the same string as the value, separated by a space, such
as L"-T ps_6_0".  This is normally interpreted as a joined form of the
"-T" option, with the value being " ps_6_0" - note the leading space in
the value.  This is not intentional and leads to confusion.

This change handles the case where an option is either the SeparateClass
or JoinedOrSeparateClass, and it appears to be joined form, starting with
spaces.  It's more than likely this is the mistake, so the spaces are
skipped, and the value starts after the spaces.  If you really want a
value that starts with space, that value can still be provided by using
the separate option: L"-Fo", L" filename-starting-with-space.dxo".

* Handle trailing spaces.

Tried resetting JoinedSpaces when value would be empty, but this leads
to errors that are still hard for user to interpret.  So this just handles
(ignores) the trailing spaces here, which doesn't seem too bad.

lib/Option/Option.cpp

@@ -110,6 +110,17 @@ Arg *Option::accept(const ArgList &Args,
                                   Twine(UnaliasedOption.getName()));
   }
 
+  // HLSL Change Start: Count spaces immediately after option name.
+  // This is to handle the case where the argument was provided as
+  // "-opt value" instead of two arguments: "-opt", "value"
+  unsigned JoinedSpaces = 0;
+  {
+    const char *Val = Args.getArgString(Index) + ArgSize;
+    while (*Val++ == ' ')
+      ++JoinedSpaces;
+  }
+  // HLSL Change End
+
   switch (getKind()) {
   case FlagClass: {
     if (ArgSize != strlen(Args.getArgString(Index)))
@@ -168,7 +179,15 @@ Arg *Option::accept(const ArgList &Args,
     // Matches iff this is an exact match.
     // FIXME: Avoid strlen.
     if (ArgSize != strlen(Args.getArgString(Index)))
-      return nullptr;
+    { // HLSL Change Begin: Except if value separated by spaces here
+      if (JoinedSpaces) {
+        const char *Value = Args.getArgString(Index) + ArgSize + JoinedSpaces;
+        if (*Value != '\0')
+          return new Arg(*this, Spelling, Index++, Value);
+      } else
+      // HLSL Change End
+        return nullptr;
+    }
 
     Index += 2;
     if (Index > Args.getNumInputArgStrings() ||
@@ -197,8 +216,11 @@ Arg *Option::accept(const ArgList &Args,
     // If this is not an exact match, it is a joined arg.
     // FIXME: Avoid strlen.
     if (ArgSize != strlen(Args.getArgString(Index))) {
-      const char *Value = Args.getArgString(Index) + ArgSize;
-      return new Arg(*this, Spelling, Index++, Value);
+      const char *Value = Args.getArgString(Index) + ArgSize
+        + JoinedSpaces; // HLSL Change: Skip spaces in joined case
+      // HLSL Change: don't interpret trailing spaces as empty value:
+      if (*Value != '\0')
+        return new Arg(*this, Spelling, Index++, Value);
     }
 
     // Otherwise it must be separate.

tools/clang/unittests/HLSL/OptionsTest.cpp

@@ -78,6 +78,8 @@ public:
   TEST_METHOD(CopyOptionsWhenSingleThenOK)
   //TEST_METHOD(CopyOptionsWhenMultipleThenOK)
 
+  TEST_METHOD(ReadOptionsJoinedWithSpacesThenOK)
+
   std::unique_ptr<DxcOpts> ReadOptsTest(const MainArgs &mainArgs,
                                         unsigned flagsToInclude,
                                         bool shouldFail = false,
@@ -306,3 +308,44 @@ TEST_F(OptionsTest, CopyOptionsWhenSingleThenOK) {
   VERIFY_ARE_NOT_EQUAL(outArgs.end(), std::find(outArgs.begin(), outArgs.end(), std::wstring(L"main")));
   VERIFY_ARE_EQUAL    (outArgs.end(), std::find(outArgs.begin(), outArgs.end(), std::wstring(L"hlsl.hlsl")));
 }
+
+TEST_F(OptionsTest, ReadOptionsJoinedWithSpacesThenOK) {
+  {
+    // Ensure parsing arguments in joined form with embedded spaces
+    // between the option and the argument works, for these argument types:
+    // - JoinedOrSeparateClass (-E, -T)
+    // - SeparateClass (-external, -external-fn)
+    const wchar_t *Args[] = {
+      L"exe.exe",   L"-E main",    L"/T  ps_6_0",
+      L"hlsl.hlsl", L"-external foo.dll", L"-external-fn  CreateObj"};
+    MainArgsArr ArgsArr(Args);
+    std::unique_ptr<DxcOpts> o = ReadOptsTest(ArgsArr, DxcFlags);
+    VERIFY_ARE_EQUAL_STR("main", o->EntryPoint.data());
+    VERIFY_ARE_EQUAL_STR("ps_6_0", o->TargetProfile.data());
+    VERIFY_ARE_EQUAL_STR("CreateObj", o->ExternalFn.data());
+    VERIFY_ARE_EQUAL_STR("foo.dll", o->ExternalLib.data());
+  }
+
+  {
+    // Ignore trailing spaces in option name for JoinedOrSeparateClass
+    // Otherwise error messages are not easy for user to interpret
+    const wchar_t *Args[] = {
+      L"exe.exe",   L"-E ", L"main",    L"/T  ", L"ps_6_0",
+      L"hlsl.hlsl"};
+    MainArgsArr ArgsArr(Args);
+    std::unique_ptr<DxcOpts> o = ReadOptsTest(ArgsArr, DxcFlags);
+    VERIFY_ARE_EQUAL_STR("main", o->EntryPoint.data());
+    VERIFY_ARE_EQUAL_STR("ps_6_0", o->TargetProfile.data());
+  }
+  {
+    // Ignore trailing spaces in option name for SeparateClass
+    // Otherwise error messages are not easy for user to interpret
+    const wchar_t *Args[] = {
+      L"exe.exe",   L"-E", L"main",    L"/T", L"ps_6_0",
+      L"hlsl.hlsl", L"-external ", L"foo.dll", L"-external-fn  ", L"CreateObj"};
+    MainArgsArr ArgsArr(Args);
+    std::unique_ptr<DxcOpts> o = ReadOptsTest(ArgsArr, DxcFlags);
+    VERIFY_ARE_EQUAL_STR("CreateObj", o->ExternalFn.data());
+    VERIFY_ARE_EQUAL_STR("foo.dll", o->ExternalLib.data());
+  }
+}