[Driver][ARM] parse version of arm/thumb architecture correctly

[j0le]

Summary

If you execute the following commandline multiple times, the behavior was 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

Most of the time the compilation succeeded, but sometimes clang reported this error:

clang++: error: the target architecture 'thumbv7em' is not supported by the target 'thumbv7em-none-windows-eabi'

The cause of the inconsistent behavior was the uninitialized variable Version.

With these commandline arguments, the variable Version was not set by getAsInteger(),
because it cannot parse a number from 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.

By using consumeInteger() instead of getAsInteger() we get 7 from the substring "7em"
and the command does not fail.

[compnerd]

Seems reasonable, though this isn't UB, its just use of an uninitialized variable.

Please add a test case.

[j0le]

I added a test case.
I hope the folder "clang/tests/Driver" is the right one.

[j0le]

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

[danielkiss]

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.

[j0le]

Changed title and summary to not contain the phrase "undefined behaviour".
Extended the test case with a test, that passes the argument "-mcpu=cortex-m1" to clang, which causes the architecture part of the triple to become "thumbv6m". This tests the body of the if statement.

[danielkiss]

Sorry for the delay, it is okay to say "ping" after a while :)
LGTM

[j0le]

In D75453#2094938, @danielkiss wrote:

Sorry for the delay, it is okay to say "ping" after a while :)
LGTM

Hi Daniel,
no problem, thanks for comming back to this review after that long time. I will say "ping" next time :)

Can you commit the patch? I don't have commit access.

[danielkiss]

@j0le, I will commit later this week for you.
@compnerd do you have any comment for this version?

[j0le]

ping

[danielkiss]

@j0le patch is landed but I just realised the review is still blocked by @compnerd. So it won't be closed I think, that is my bad.

https://github.com/llvm/llvm-project/commit/070acb1d1e51ffd289a46b8f93e993635d0053b7

clang/lib/Driver/ToolChains/Clang.cpp

std::string NormalizedTriple =
             .normalize();
     CmdArgs.push_back("-aux-triple");
     CmdArgs.push_back(Args.MakeArgString(NormalizedTriple));
   }
 
   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;
   }
 
   // Push all default warning arguments that are specific to
   // the given target.  These come before user provided warning options
   // are provided.
   TC.addClangWarningOptions(CmdArgs);

clang/test/Driver/windows-thumbv7em.cpp

+// RUN: %clang -target thumb-none-windows-eabi-coff -mcpu=cortex-m7 -### -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix CHECK-V7
+// CHECK-V7-NOT: error: the target architecture 'thumbv7em' is not supported by the target 'thumbv7em-none-windows-eabi'
+
+// RUN: %clang -target thumb-none-windows-eabi-coff -mcpu=cortex-m1 -### -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix CHECK-V6
+// CHECK-V6: error: the target architecture 'thumbv6m' is not supported by the target 'thumbv6m-none-windows-eabi'
+