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

[LLD] [MinGW] Sync --thinlto-cache-dir option details with ELF #77010

Merged
merged 1 commit into from Jan 8, 2024

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Jan 4, 2024

Disallow using the form with a separate argument,
"--thinlto-cache-dir dir", allow only the one with equals, "--thintlo-cache-dir=dir". This is the only form that actually was tested when this was added in
f794808, and matches the ELF side, where only the form with an equals is supported (and this was also the case at the time when this option was added to the MinGW linker).

Disallow using the form with a separate argument,
"--thinlto-cache-dir dir", allow only the one with equals,
"--thintlo-cache-dir=dir". This is the only form that actually
was tested when this was added in
f794808, and matches the ELF
side, where only the form with an equals is supported (and this
was also the case at the time when this option was added to the
MinGW linker).
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lld

Author: Martin Storsjö (mstorsjo)

Changes

Disallow using the form with a separate argument,
"--thinlto-cache-dir dir", allow only the one with equals, "--thintlo-cache-dir=dir". This is the only form that actually was tested when this was added in
f794808, and matches the ELF side, where only the form with an equals is supported (and this was also the case at the time when this option was added to the MinGW linker).


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

1 Files Affected:

  • (modified) lld/MinGW/Options.td (+2-2)
diff --git a/lld/MinGW/Options.td b/lld/MinGW/Options.td
index d4a49cdbd53593..d8471d5a7bc9ed 100644
--- a/lld/MinGW/Options.td
+++ b/lld/MinGW/Options.td
@@ -186,8 +186,8 @@ def appcontainer: F<"appcontainer">, HelpText<"Set the appcontainer flag in the
 defm delayload: Eq<"delayload", "DLL to load only on demand">;
 defm mllvm: EqNoHelp<"mllvm">;
 defm pdb: Eq<"pdb", "Output PDB debug info file, chosen implicitly if the argument is empty">;
-defm thinlto_cache_dir: EqLong<"thinlto-cache-dir",
-  "Path to ThinLTO cached object file directory">;
+def thinlto_cache_dir: JJ<"thinlto-cache-dir=">,
+  HelpText<"Path to ThinLTO cached object file directory">;
 defm Xlink : Eq<"Xlink", "Pass <arg> to the COFF linker">, MetaVarName<"<arg>">;
 defm guard_cf : B<"guard-cf", "Enable Control Flow Guard" ,
   "Do not enable Control Flow Guard (default)">;

@cjacek
Copy link
Contributor

cjacek commented Jan 8, 2024

The patch looks good to me, but should it have a test case? I guess it doesn't matter much in this case, but it also seems easy enough to have one.

@mstorsjo
Copy link
Member Author

mstorsjo commented Jan 8, 2024

The patch looks good to me, but should it have a test case? I guess it doesn't matter much in this case, but it also seems easy enough to have one.

Not sure we explicitly want to have a test to disallow specific spellings of all potential options (unless we have a real world case for that) - the main reason here is that when looking into adding more options from lld/ELF into lld/MinGW, I noticed this discrepancy - which seems unintentional (all uses of the option use this one single form).

Copy link
Contributor

@cjacek cjacek left a comment

Choose a reason for hiding this comment

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

Sure, fair enough, I wasn't sure how strict is the general rule that functional changes should have a test case. Thanks!

@mstorsjo mstorsjo merged commit f84bfa2 into llvm:main Jan 8, 2024
6 checks passed
@mstorsjo mstorsjo deleted the lld-thinlto-cache-dir branch January 8, 2024 22:10
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…77010)

Disallow using the form with a separate argument,
"--thinlto-cache-dir dir", allow only the one with equals,
"--thintlo-cache-dir=dir". This is the only form that actually was
tested when this was added in
f794808, and matches the ELF side,
where only the form with an equals is supported (and this was also the
case at the time when this option was added to the MinGW linker).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants