[ARM] Adding diagnostics for mcmodel=tiny when used in invalid targets #125643
Conversation
|
@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
|
Can you add tests and a release note entry? Thanks! |
Thank you for the response! |
hstk30-hw
changed the title
llvmbot
added
the
clang:driver
@cor3ntin I have addressed your comment. Could you please look into it. Thank you!! |
|
ping @cor3ntin |
|
ping @hstk30-hw |
|
ping @cor3ntin @hstk30-hw |
|
Hi, these is a pending review suggestion. Just put the test cast to exist test file, maybe like clang/test/Driver/mcmodel.c |
|
ping @cor3ntin |
1 similar comment
|
ping @cor3ntin |
|
@hstk30-hw I have been waiting for approval from @cor3ntin, the PR has been open for over 2 months now, can we go ahead and merge this if it's alright? |
There was a problem hiding this comment.
We typically add such incompatibility check in clang/lib/Driver instead of clang/lib/Frontend.
(Apologies, I will have limited internet access between April 20th and May 4th, and my response time may be delayed.. Happy if a regular clang maintainer is happy.)
|
This is really not my area of expertise. @AaronBallman Who would be a good reviewer here? |
| 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(); |
There was a problem hiding this comment.
| 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
| @@ -240,6 +240,9 @@ Changes to Sanitizers | |||
| Other Changes | |||
| ------------- | |||
|
|
|||
| * The -mcmodel=tiny option for the x86 architecture now triggers a frontend diagnostic. | |||
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
| @@ -240,6 +240,9 @@ Changes to Sanitizers | |||
| Other Changes | |||
| ------------- | |||
|
|
|||
| * The -mcmodel=tiny option for the x86 architecture now triggers a frontend diagnostic. | |||
There was a problem hiding this comment.
| * 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
| @@ -4,6 +4,7 @@ | |||
| // 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 | |||
There was a problem hiding this comment.
What about Line 2 of this file? That seems like it would now start failing, right?
There was a problem hiding this comment.
Thanks for the response, I have modified the command.
Thanks for the response, I have made the changes in clang/lib/Driver |
| @@ -44,3 +44,5 @@ | |||
| // ERR-AARCH64_32: error: unsupported argument 'small' to option '-mcmodel=' for target 'aarch64_32-unknown-linux' | |||
|
|
|||
| // ERR-LOONGARCH64-PLT-EXTREME: error: invalid argument '-mcmodel=extreme' not allowed with '-fplt' | |||
|
|
|||
| // CHECK: error: unsupported argument 'tiny' to option '-mcmodel=' for target '{{.*}}' | |||
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.
| @@ -1,5 +1,5 @@ | |||
| // RUN: not %clang -### -c --target=i686 -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=ERR-MEDIUM %s | |||
| // RUN: %clang --target=x86_64 -### -c -mcmodel=tiny %s 2>&1 | FileCheck --check-prefix=TINY %s | |||
| // RUN: not %clang --target=x86_64 -### -c -mcmodel=tiny %s 2>&1 | FileCheck --check-prefix=TINY %s | |||
There was a problem hiding this comment.
I'm surprised this test is passing. Should this not be using the check prefix ERR-TINY now because we expect an error?
| @@ -1755,6 +1755,18 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) { | |||
| << TC.getTriple().str(); | |||
| } | |||
|
|
|||
| // Throw diagnosis if mcmodel=tiny option is passed for targets other than ARM | |||
| // or AArch64. | |||
| if (Arg *A = UArgs->getLastArg(options::OPT_mcmodel_EQ)) { | |||
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.
There was a problem hiding this comment.
tiny is expected to work with AArch64.
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 fix adds diagnostics to ensure the compiler throws a frontend error instead of crashing with an internal error.
Reduced test case: