Page MenuHomePhabricator

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

Authored by j0le on Mar 2 2020, 6:51 AM.

Details

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.

Diff Detail

Event Timeline

j0le created this revision.Mar 2 2020, 6:51 AM
compnerd requested changes to this revision.Apr 1 2020, 10:01 PM

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

Please add a test case.

This revision now requires changes to proceed.Apr 1 2020, 10:01 PM
j0le updated this revision to Diff 255354.Apr 6 2020, 8:22 AM

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

j0le added a comment.Apr 24 2020, 4:48 AM

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

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 updated this revision to Diff 260912.Wed, Apr 29, 7:47 AM
j0le retitled this revision from [Driver][ARM] fix undefined behaviour when checking architecture version to [Driver][ARM] parse version of arm/thumb architecture correctly.
j0le edited the summary of this revision. (Show Details)

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.