Skip to content
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

[clang-cl] [Driver] Fix clang-cl driver supported colon options #88216

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

MaxEW707
Copy link
Contributor

@MaxEW707 MaxEW707 commented Apr 9, 2024

Tested locally against msvc 1939.
MSVC supports the colon form of various options that take a path even though that isn't documented for all options on MSDN.
I added the colon form for all supported msvc options that clang-cl does not intentionally ignore due to being unsupported.

I also fixed the existing colon options by ensure we use JoinedOrSeparate. /F[x] argument must used within the same command line argument. However, /F[x]: arguments are allowed to span a command line argument. The following is valid /F[x]: path which is 2 separate command line arguments.
You can see this documented here, https://learn.microsoft.com/en-us/cpp/build/reference/fo-object-file-name?view=msvc-170, where it says /Fo:[ ]"pathname".
This behaviour works for all colon options that take a path and I tested it locally against msvc 1939 for all the option changes in this PR.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Apr 10, 2024
@MaxEW707
Copy link
Contributor Author

CC @rnk

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 10, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Max Winkler (MaxEW707)

Changes

Tested locally against msvc 1939.
MSVC supports the colon form of various options that take a path even though that isn't documented for all options on MSDN.
I added the colon form for all supported msvc options that clang-cl does not intentionally ignore due to being unsupported.

I also fixed the existing colon options by ensure we use JoinedOrSeparate. /F[x] argument must used within the same command line argument. However, /F[x]: arguments are allowed to span a command line argument. The following is valid /F[x]: path which is 2 separate command line arguments.
You can see this documented here, https://learn.microsoft.com/en-us/cpp/build/reference/fo-object-file-name?view=msvc-170, where it says /Fo:[ ]"pathname".
This behaviour works for all colon options that take a path and I tested it locally against msvc 1939 for all the option changes in this PR.


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

3 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+4-2)
  • (modified) clang/test/Driver/cl-outputs.c (+4)
  • (modified) clang/test/Driver/cl-pch.cpp (+6)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index f745e573eb2686..bc7041c7830984 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8331,14 +8331,15 @@ def _SLASH_FI : CLJoinedOrSeparate<"FI">,
 def _SLASH_Fe : CLJoined<"Fe">,
   HelpText<"Set output executable file name">,
   MetaVarName<"<file or dir/>">;
-def _SLASH_Fe_COLON : CLJoined<"Fe:">, Alias<_SLASH_Fe>;
+def _SLASH_Fe_COLON : CLJoinedOrSeparate<"Fe:">, Alias<_SLASH_Fe>;
 def _SLASH_Fi : CLCompileJoined<"Fi">,
   HelpText<"Set preprocess output file name (with /P)">,
   MetaVarName<"<file>">;
+def _SLASH_Fi_COLON : CLJoinedOrSeparate<"Fi:">, Alias<_SLASH_Fi>;
 def _SLASH_Fo : CLCompileJoined<"Fo">,
   HelpText<"Set output object file (with /c)">,
   MetaVarName<"<file or dir/>">;
-def _SLASH_Fo_COLON : CLCompileJoined<"Fo:">, Alias<_SLASH_Fo>;
+def _SLASH_Fo_COLON : CLCompileJoinedOrSeparate<"Fo:">, Alias<_SLASH_Fo>;
 def _SLASH_guard : CLJoined<"guard:">,
   HelpText<"Enable Control Flow Guard with /guard:cf, or only the table with /guard:cf,nochecks. "
            "Enable EH Continuation Guard with /guard:ehcont">;
@@ -8433,6 +8434,7 @@ def _SLASH_Zc_dllexportInlines_ : CLFlag<"Zc:dllexportInlines-">,
   HelpText<"Do not dllexport/dllimport inline member functions of dllexport/import classes">;
 def _SLASH_Fp : CLJoined<"Fp">,
   HelpText<"Set pch file name (with /Yc and /Yu)">, MetaVarName<"<file>">;
+def _SLASH_Fp_COLON : CLJoinedOrSeparate<"Fp:">, Alias<_SLASH_Fp>;
 
 def _SLASH_Gd : CLFlag<"Gd">,
   HelpText<"Set __cdecl as a default calling convention">;
diff --git a/clang/test/Driver/cl-outputs.c b/clang/test/Driver/cl-outputs.c
index 4d58f0fb548b57..4298657ac49f5c 100644
--- a/clang/test/Driver/cl-outputs.c
+++ b/clang/test/Driver/cl-outputs.c
@@ -118,6 +118,7 @@
 
 // RUN: %clang_cl /Fefoo.ext -### -- %s 2>&1 | FileCheck -check-prefix=FeEXT %s
 // RUN: %clang_cl /Fe:foo.ext -### -- %s 2>&1 | FileCheck -check-prefix=FeEXT %s
+// RUN: %clang_cl /Fe: foo.ext -### -- %s 2>&1 | FileCheck -check-prefix=FeEXT %s
 // FeEXT: "-out:foo.ext"
 
 // RUN: %clang_cl /LD /Fefoo.ext -### -- %s 2>&1 | FileCheck -check-prefix=FeEXTDLL %s
@@ -270,6 +271,8 @@
 // P: "-o" "cl-outputs.i"
 
 // RUN: %clang_cl /P /Fifoo -### -- %s 2>&1 | FileCheck -check-prefix=Fi1 %s
+// RUN: %clang_cl /P /Fi:foo -### -- %s 2>&1 | FileCheck -check-prefix=Fi1 %s
+// RUN: %clang_cl /P /Fi: foo -### -- %s 2>&1 | FileCheck -check-prefix=Fi1 %s
 // Fi1: "-E"
 // Fi1: "-o" "foo.i"
 
@@ -302,6 +305,7 @@
 // RELATIVE_OBJPATH1: "-object-file-name=a.obj"
 
 // RUN: %clang_cl -fdebug-compilation-dir=. /Z7 /Fo:a.obj -### -- %s 2>&1 | FileCheck -check-prefix=RELATIVE_OBJPATH1_COLON %s
+// RUN: %clang_cl -fdebug-compilation-dir=. /Z7 /Fo: a.obj -### -- %s 2>&1 | FileCheck -check-prefix=RELATIVE_OBJPATH1_COLON %s
 // RELATIVE_OBJPATH1_COLON: "-object-file-name=a.obj"
 
 // RUN: %clang_cl -fdebug-compilation-dir=. /Z7 /Fofoo/a.obj -### -- %s 2>&1 | FileCheck -check-prefix=RELATIVE_OBJPATH2 %s
diff --git a/clang/test/Driver/cl-pch.cpp b/clang/test/Driver/cl-pch.cpp
index d09b177eb617de..cc4fc435a61c20 100644
--- a/clang/test/Driver/cl-pch.cpp
+++ b/clang/test/Driver/cl-pch.cpp
@@ -99,6 +99,12 @@
 // /Yu /Fpout.pch => out.pch is filename
 // RUN: %clang_cl -Werror /Yupchfile.h /FIpchfile.h /Fpout.pch /c -### -- %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-YUFP1 %s
+// /Yu /Fp:out.pch => out.pch is filename
+// RUN: %clang_cl -Werror /Yupchfile.h /FIpchfile.h /Fp:out.pch /c -### -- %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-YUFP1 %s
+// /Yu /Fp: out.pch => out.pch is filename
+// RUN: %clang_cl -Werror /Yupchfile.h /FIpchfile.h /Fp: out.pch /c -### -- %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-YUFP1 %s
 // Use .pch file, but don't build it.
 // CHECK-YUFP1: -include-pch
 // CHECK-YUFP1: out.pch

Copy link
Collaborator

@rnk rnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for polishing this corner of the driver interface! It's interesting that they have an alternative separate spelling. I always felt like the /Fopath.cpp pattern was a bit unreadable.

@MaxEW707
Copy link
Contributor Author

Thank you for polishing this corner of the driver interface! It's interesting that they have an alternative separate spelling. I always felt like the /Fopath.cpp pattern was a bit unreadable.

Thanks for the review. I will need you to commit on my behalf. I think its about time I figure out what conditions I need to meet in-order to get commit after approval rights.

/Fopath.cpp is super unreadable. Unfortunately, I don't remember when the colon spelling came into existence but anything from VS2017 definitely has the colon alternative spelling.

@rnk rnk merged commit 3f2371e into llvm:main Apr 18, 2024
7 checks passed
@MaxEW707 MaxEW707 deleted the mew/fix-cl-driver-colon-opts branch April 19, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants