[PATCH] D75453: [Driver][ARM] fix undefined behaviour when checking architecture version
danielkiss added a comment. Please update the title and the summary because those will be the commit message. IMHO the test a case would be extended with a test where the body of the modified if condition is tested. Otherwise LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75453/new/ https://reviews.llvm.org/D75453 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75453: [Driver][ARM] fix undefined behaviour when checking architecture version
j0le added a comment. I would like to ask, whether I could "Accept the Revision" myself, or is this only allowed to be done by code owners or other reviewers? And when the revision is accepted, do I have to commit this, or can this only be done by people who have commit access or is this done automatically? Many thanks in advance, Ole CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75453/new/ https://reviews.llvm.org/D75453 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75453: [Driver][ARM] fix undefined behaviour when checking architecture version
j0le updated this revision to Diff 255354.
j0le added a comment.
I added a test case.
I hope the folder "clang/tests/Driver" is the right one.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75453/new/
https://reviews.llvm.org/D75453
Files:
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/windows-thumbv7em.cpp
Index: clang/test/Driver/windows-thumbv7em.cpp
===
--- /dev/null
+++ clang/test/Driver/windows-thumbv7em.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cpp --target=thumbv7em-none-windows-eabi-coff \
+// RUN: -mcpu=cortex-m7 -c %s -o /dev/null 2>&1 \
+// RUN: | FileCheck --allow-empty %s
+
+// CHECK-NOT: error: the target architecture 'thumbv7em' is not supported by
the target 'thumbv7em-none-windows-eabi'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4091,9 +4091,10 @@
if (Triple.isOSWindows() && (Triple.getArch() == llvm::Triple::arm ||
Triple.getArch() == llvm::Triple::thumb)) {
unsigned Offset = Triple.getArch() == llvm::Triple::arm ? 4 : 6;
-unsigned Version;
-Triple.getArchName().substr(Offset).getAsInteger(10, Version);
-if (Version < 7)
+unsigned Version = 0;
+bool Failure =
+Triple.getArchName().substr(Offset).consumeInteger(10, Version);
+if (Failure || Version < 7)
D.Diag(diag::err_target_unsupported_arch) << Triple.getArchName()
<< TripleStr;
}
Index: clang/test/Driver/windows-thumbv7em.cpp
===
--- /dev/null
+++ clang/test/Driver/windows-thumbv7em.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cpp --target=thumbv7em-none-windows-eabi-coff \
+// RUN: -mcpu=cortex-m7 -c %s -o /dev/null 2>&1 \
+// RUN: | FileCheck --allow-empty %s
+
+// CHECK-NOT: error: the target architecture 'thumbv7em' is not supported by the target 'thumbv7em-none-windows-eabi'
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4091,9 +4091,10 @@
if (Triple.isOSWindows() && (Triple.getArch() == llvm::Triple::arm ||
Triple.getArch() == llvm::Triple::thumb)) {
unsigned Offset = Triple.getArch() == llvm::Triple::arm ? 4 : 6;
-unsigned Version;
-Triple.getArchName().substr(Offset).getAsInteger(10, Version);
-if (Version < 7)
+unsigned Version = 0;
+bool Failure =
+Triple.getArchName().substr(Offset).consumeInteger(10, Version);
+if (Failure || Version < 7)
D.Diag(diag::err_target_unsupported_arch) << Triple.getArchName()
<< TripleStr;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75453: [Driver][ARM] fix undefined behaviour when checking architecture version
compnerd requested changes to this revision. compnerd added a comment. This revision now requires changes to proceed. Herald added a subscriber: danielkiss. Seems reasonable, though this isn't UB, its just use of an uninitialized variable. Please add a test case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75453/new/ https://reviews.llvm.org/D75453 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75453: [Driver][ARM] fix undefined behaviour when checking architecture version
j0le created this revision.
j0le added a reviewer: compnerd.
j0le added a project: clang.
Herald added a subscriber: kristof.beyls.
Hello everyone,
this is my first patch to/for clang. I hope, I did everything right. If not,
please tell me.
I found this bug:
If you execute the following commandline multiple times, the behavior is not
always the same:
clang++ --target=thumbv7em-none-windows-eabi-coff -march=armv7-m
-mcpu=cortex-m7 -o temp.obj -c -x c++ empty.cpp
where empty.cpp is an empty file in the current working directory.
Most of the time the compilation succeeds, but sometimes clang reports this
error:
clang++: error: the target architecture 'thumbv7em' is not supported by the
target 'thumbv7em-none-windows-eabi'
With these commandline arguments, the variable Version is not set by
getAsInteger() (see diff),
because it does not parse the substring "7em" of "thumbv7em".
To get a consistent behaviour, it's enough to initialize the variable Version
to zero.
(Zero is smaller than 7, so the comparison will be true.)
Then the command always fails with the error message seen above.
I don't know, if this is the intended behaviour.
But if it isn't, I would suggest to use the function consumeInteger() instead.
And check the return value of course.
consumeInteger() is able to get 7 from "7em".
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75453
Files:
clang/lib/Driver/ToolChains/Clang.cpp
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3531,9 +3531,10 @@
if (Triple.isOSWindows() && (Triple.getArch() == llvm::Triple::arm ||
Triple.getArch() == llvm::Triple::thumb)) {
unsigned Offset = Triple.getArch() == llvm::Triple::arm ? 4 : 6;
-unsigned Version;
-Triple.getArchName().substr(Offset).getAsInteger(10, Version);
-if (Version < 7)
+unsigned Version = 0;
+bool Failure =
+Triple.getArchName().substr(Offset).consumeInteger(10, Version);
+if (Failure || Version < 7)
D.Diag(diag::err_target_unsupported_arch) << Triple.getArchName()
<< TripleStr;
}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3531,9 +3531,10 @@
if (Triple.isOSWindows() && (Triple.getArch() == llvm::Triple::arm ||
Triple.getArch() == llvm::Triple::thumb)) {
unsigned Offset = Triple.getArch() == llvm::Triple::arm ? 4 : 6;
-unsigned Version;
-Triple.getArchName().substr(Offset).getAsInteger(10, Version);
-if (Version < 7)
+unsigned Version = 0;
+bool Failure =
+Triple.getArchName().substr(Offset).consumeInteger(10, Version);
+if (Failure || Version < 7)
D.Diag(diag::err_target_unsupported_arch) << Triple.getArchName()
<< TripleStr;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
