[Driver] Reject -mcmodel=tiny on X86#125643
Merged
Conversation
Member
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: None (ShashwathiNavada) ChangesThe mcmodel=tiny memory model is only valid on ARM targets. While trying this on X86 compiler throws an internal error along with stack dump. #125641 Full diff: https://github.com/llvm/llvm-project/pull/125643.diff 1 Files Affected:
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 11fd6ab7f52a795..ac8d8be572012bb 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -1897,6 +1897,15 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
Opts.setInlining(CodeGenOptions::NormalInlining);
}
+// -mcmodel option.
+if (const llvm::opt::Arg *A = Args.getLastArg(clang::driver::options::OPT_mcmodel_EQ))
+{
+ llvm::StringRef modelName = A->getValue();
+ if(modelName=="tiny" && !T.isARM())
+ Diags.Report(diag::err_drv_unsupported_option_argument_for_target)
+ << A->getSpelling() <<modelName<< T.getTriple();
+}
+
// PIC defaults to -fno-direct-access-external-data while non-PIC defaults to
// -fdirect-access-external-data.
Opts.DirectAccessExternalData =
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ShashwathiNavada
changed the title
Contributor
|
Can you add tests and a release note entry? Thanks! |
Contributor
Author
Thank you for the response! |
hstk30-hw
changed the title
llvmbot
added
the
clang:driver
Contributor
Author
@cor3ntin I have addressed your comment. Could you please look into it. Thank you!! |
Contributor
Author
|
ping @cor3ntin |
Contributor
Author
|
ping @hstk30-hw |
Contributor
Author
|
ping @cor3ntin @hstk30-hw |
Contributor
|
Hi, these is a pending review suggestion. Just put the test cast to exist test file, maybe like clang/test/Driver/mcmodel.c |
Comment on lines
1911
to
1914
| llvm::StringRef modelName = A->getValue(); | ||
| if (modelName == "tiny" && !(T.isARM() || T.isAArch64())) { | ||
| Diags.Report(diag::err_drv_unsupported_option_argument_for_target) | ||
| << A->getSpelling() << modelName << T.getTriple(); |
Collaborator
There was a problem hiding this comment.
Suggested change
| llvm::StringRef modelName = A->getValue(); | |
| if (modelName == "tiny" && !(T.isARM() || T.isAArch64())) { | |
| Diags.Report(diag::err_drv_unsupported_option_argument_for_target) | |
| << A->getSpelling() << modelName << T.getTriple(); | |
| StringRef ModelName = A->getValue(); | |
| if (ModelName == "tiny" && !(T.isARM() || T.isAArch64())) { | |
| Diags.Report(diag::err_drv_unsupported_option_argument_for_target) | |
| << A->getSpelling() << ModelName << T.getTriple(); |
Fixing for our coding style.
llvm/docs/ReleaseNotes.md
Outdated
| Other Changes | ||
| ------------- | ||
|
|
||
| * The -mcmodel=tiny option for the x86 architecture now triggers a frontend diagnostic. |
Collaborator
There was a problem hiding this comment.
Oops, this belongs in clang/docs/ReleaseNotes.rst rather than in LLVM (because this is changing diagnostic behavior of the Clang driver).
llvm/docs/ReleaseNotes.md
Outdated
| Other Changes | ||
| ------------- | ||
|
|
||
| * The -mcmodel=tiny option for the x86 architecture now triggers a frontend diagnostic. |
Collaborator
There was a problem hiding this comment.
Suggested change
| * The -mcmodel=tiny option for the x86 architecture now triggers a frontend diagnostic. | |
| * The ``-mcmodel=tiny`` option will now be diagnosed on all targets other than ARM or AArch64. |
clang/test/Driver/mcmodel.c
Outdated
| // RUN: %clang --target=x86_64 -### -S -mcmodel=kernel %s 2>&1 | FileCheck --check-prefix=KERNEL %s | ||
| // RUN: %clang --target=x86_64 -### -c -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=MEDIUM %s | ||
| // RUN: %clang --target=x86_64 -### -S -mcmodel=large %s 2>&1 | FileCheck --check-prefix=LARGE %s | ||
| // RUN: not %clang --target=x86_64 -c -mcmodel=tiny %s 2>&1 | FileCheck %s |
Collaborator
There was a problem hiding this comment.
What about Line 2 of this file? That seems like it would now start failing, right?
Contributor
Author
There was a problem hiding this comment.
Thanks for the response, I have modified the command.
Contributor
Author
Thanks for the response, I have made the changes in clang/lib/Driver |
clang/test/Driver/mcmodel.c
Outdated
|
|
||
| // ERR-LOONGARCH64-PLT-EXTREME: error: invalid argument '-mcmodel=extreme' not allowed with '-fplt' | ||
|
|
||
| // CHECK: error: unsupported argument 'tiny' to option '-mcmodel=' for target '{{.*}}' |
Collaborator
There was a problem hiding this comment.
We have nothing with a check prefix of just CHECK so this will never actually be tested. Also, this is the same message as ERR-TINY so we wouldn't need this line anyway.
MaskRay
reviewed
Apr 16, 2025
clang/lib/Driver/Driver.cpp
Outdated
|
|
||
| // Throw diagnosis if mcmodel=tiny option is passed for targets other than ARM | ||
| // or AArch64. | ||
| if (Arg *A = UArgs->getLastArg(options::OPT_mcmodel_EQ)) { |
Member
There was a problem hiding this comment.
Move to clang/lib/Driver/ToolChains/CommonArgs.cpp addMCModel. You might delete add an early return to that function and run check-clang-driver to find relevant tests. Then follow that style and add one for Arm.
Member
There was a problem hiding this comment.
tiny is expected to work with AArch64.
Contributor
Author
There was a problem hiding this comment.
@MaskRay Thank you for your response!
I noticed that in the function you mentioned, there’s a check like this:
else if (Triple.getArch() == llvm::Triple::x86_64) {
Ok = llvm::is_contained({"small", "kernel", "medium", "large", "tiny"}, CM);
}
I found that removing the tiny option here gives the desired result.
Would you mind confirming if this approach looks good?
Contributor
Author
|
ping @MaskRay |
MaskRay
reviewed
May 6, 2025
clang/docs/ReleaseNotes.rst
Outdated
|
|
||
|
|
||
| - An error is now emitted when a ``musttail`` call is made to a function marked with the ``not_tail_called`` attribute. (#GH133509). | ||
| - The ``-mcmodel=tiny`` option will now be diagnosed on all targets other than ARM or AArch64. |
Member
There was a problem hiding this comment.
The commit message/description and the note should be changed. Only X86 is affected. This change is pretty niche and does not deserve a release note.
Contributor
Author
There was a problem hiding this comment.
@MaskRay, Thank you for the response. I have removed the release note entry and updated the commit message as well.
ShashwathiNavada
changed the title
MaskRay
changed the title
MaskRay
approved these changes
May 7, 2025
Contributor
|
This patch causes a test fail on I tested with build: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/31001 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/27305 Here is the relevant piece of the build log for the reference |
Contributor
|
Hi there, we would appreciate it if you could revert this PR while investigating. It breaks our buildbot and will block our downstream merge. |
Contributor
|
I put up #138959 which removes the offending |
jplehr
added a commit
that referenced
this pull request
May 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The mcmodel=tiny memory model is only valid on ARM targets. While trying this on X86 compiler throws an internal error along with stack dump. #125641
This patch resolves the issue.
Reduced test case: