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] Allow a colon after /Fo option #87209

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Conversation

russelltg
Copy link
Contributor

Modeled after 8513a68

According to https://learn.microsoft.com/en-us/cpp/build/reference/fo-object-file-name?view=msvc-170, /Fo accepts a trailing-colon variant. This is also tested in practice. This allows clang-cl to parse this.

I just copied one of the existing tests, let me know if this is not the best way to do this. I tested that the test does not pass beofre the Options.td change, and that it does after.

See also #46065

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Mar 31, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 31, 2024

@llvm/pr-subscribers-clang-driver

Author: Russell Greene (russelltg)

Changes

Modeled after 8513a68

According to https://learn.microsoft.com/en-us/cpp/build/reference/fo-object-file-name?view=msvc-170, /Fo accepts a trailing-colon variant. This is also tested in practice. This allows clang-cl to parse this.

I just copied one of the existing tests, let me know if this is not the best way to do this. I tested that the test does not pass beofre the Options.td change, and that it does after.

See also #46065


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

2 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+1)
  • (modified) clang/test/Driver/cl-outputs.c (+3)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 04eb87f0d5d1aa..f5289fb00c895e 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8313,6 +8313,7 @@ def _SLASH_Fi : CLCompileJoined<"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_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">;
diff --git a/clang/test/Driver/cl-outputs.c b/clang/test/Driver/cl-outputs.c
index 07ff43642a62be..4d58f0fb548b57 100644
--- a/clang/test/Driver/cl-outputs.c
+++ b/clang/test/Driver/cl-outputs.c
@@ -301,5 +301,8 @@
 // RUN: %clang_cl -fdebug-compilation-dir=. /Z7 /Foa.obj -### -- %s 2>&1 | FileCheck -check-prefix=RELATIVE_OBJPATH1 %s
 // 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
+// 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
 // RELATIVE_OBJPATH2: "-object-file-name=foo\\a.obj"

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 31, 2024

@llvm/pr-subscribers-clang

Author: Russell Greene (russelltg)

Changes

Modeled after 8513a68

According to https://learn.microsoft.com/en-us/cpp/build/reference/fo-object-file-name?view=msvc-170, /Fo accepts a trailing-colon variant. This is also tested in practice. This allows clang-cl to parse this.

I just copied one of the existing tests, let me know if this is not the best way to do this. I tested that the test does not pass beofre the Options.td change, and that it does after.

See also #46065


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

2 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+1)
  • (modified) clang/test/Driver/cl-outputs.c (+3)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 04eb87f0d5d1aa..f5289fb00c895e 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8313,6 +8313,7 @@ def _SLASH_Fi : CLCompileJoined<"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_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">;
diff --git a/clang/test/Driver/cl-outputs.c b/clang/test/Driver/cl-outputs.c
index 07ff43642a62be..4d58f0fb548b57 100644
--- a/clang/test/Driver/cl-outputs.c
+++ b/clang/test/Driver/cl-outputs.c
@@ -301,5 +301,8 @@
 // RUN: %clang_cl -fdebug-compilation-dir=. /Z7 /Foa.obj -### -- %s 2>&1 | FileCheck -check-prefix=RELATIVE_OBJPATH1 %s
 // 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
+// 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
 // RELATIVE_OBJPATH2: "-object-file-name=foo\\a.obj"

@MaxEW707
Copy link
Contributor

MaxEW707 commented Apr 1, 2024

@rnk for review.

LGTM!

From experience with msvc the docs are very much outdated. For example almost all path options accept the : form.
For example /Fd: is valid even though the msdn docs do not state that. https://learn.microsoft.com/en-us/cpp/build/reference/fd-program-database-file-name?view=msvc-170.
Try out the following and it works, cl.exe /std:c++17 /Fo:test.obj /Fd:test.pdb /Zi /c test.cpp.

Thankfully from memory VS2019+ /? lists all the options that accept a colon form.
The output of msvc 1939 /? shows the following:

/Fd: <file> name .PDB file              /Fe: <file> name executable file
/Fm: <file> name map file               /Fo: <file> name object file
/Fp: <file> name .PCH file              /FR: <file> name extended .SBR file
/Fi: <file> name preprocessed file

showing that these options do in fact accept the colon form even though MSDN is outdated.

I think it would be super appreciated in this PR if we can fix all instances where clang-cl fails to accept the colon form for any non-ignored options that are supported if you got the time. If not no worries it was on my backlog anyways when I get time heh :).

@rnk
Copy link
Collaborator

rnk commented Apr 1, 2024

That's interesting! I wonder how long the /F flags have accepted trailing colons. That form seems a lot more readable.

I went looking for examples of how to do this with fewer aliases, but it looks like this is what is done currently:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td#L5650

The only other prior art I could identify was the JoinedOrSeparate kind, which we could generalize to "JoinedOrEq" or "JoinedOrColon" in tablegen, and that would be the most maintainable, most general fix I can think of.

Regardless, I'm happy to approve this and other future patches to add the aliases in the meantime.

@rnk rnk merged commit 5ff2773 into llvm:main Apr 1, 2024
7 checks passed
@russelltg russelltg deleted the clang_cl_fo_colon branch April 1, 2024 18:29
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.

4 participants