This is an archive of the discontinued LLVM Phabricator instance.

[BOLT][NFC] Fix undefined behavior in encodeAnnotationImm
ClosedPublic

Authored by Amir on Feb 21 2022, 9:38 AM.

Details

Summary

Fix UBSan-reported issue in MCPlusBuilder::encodeAnnotationImm (left shift of a
negative value).

Test Plan:

ninja check-bolt
...
PASS: BOLT-Unit :: Core/./CoreTests/AArch64/MCPlusBuilderTester.Annotation/0 (1 of 140)
PASS: BOLT-Unit :: Core/./CoreTests/X86/MCPlusBuilderTester.Annotation/0 (131 of 134)

Diff Detail

Event Timeline

Amir created this revision.Feb 21 2022, 9:38 AM
Amir requested review of this revision.Feb 21 2022, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2022, 9:38 AM
yota9 added inline comments.Feb 21 2022, 9:49 AM
bolt/include/bolt/Core/MCPlusAnnotation.h
21

May I suggest with replacing assert with if condition? I think it is the important thing that we should check with disabled assertions too.

Amir added inline comments.Feb 21 2022, 10:58 AM
bolt/include/bolt/Core/MCPlusAnnotation.h
21

Good point.

Amir updated this revision to Diff 410629.Feb 22 2022, 1:17 PM

Check annotation value in release builds

yota9 added a comment.Feb 22 2022, 1:20 PM

Sorry, why the index assert has gone? It is unlikely to reach this limit, but why not to check it anyway :)

Amir added a comment.Feb 22 2022, 1:23 PM

Sorry, why the index assert has gone? It is unlikely to reach this limit, but why not to check it anyway :)

It's gone because I changed Index type from unsigned to uint8_t. We're only using 1 byte of data, no need to pass larger type.

yota9 added a comment.Feb 22 2022, 1:26 PM

@Amir Sorry, missed that since the extractAnnotationIndex and the test are still using unsigned values type. Could you please fix that?

Amir added a comment.Feb 22 2022, 1:26 PM

I wanted to test MCPlusBuilder’s private methods (encodeAnnotationImm and friends) that are not directly exposed through public methods. The options were:

  1. add test class as a friend of MCPlusBuilder (less invasive, future unit tests may follow)
  2. move these methods into another file (eg MCPlusAnnotation.h) and include it into MCPlusBuilder.h and test source, the downside is code churn and pollution of the global namespace with these new functions,
  3. test using hackish indirect way (through GnuArgsSize annotation) that doesn’t require any changes to MCPlusBuilder source, does the job but test code would be ugly.

This diff goes with option 2, but I'm open to the feedback.

Amir updated this revision to Diff 410636.Feb 22 2022, 1:37 PM

Change extractAnnotationIndex return type from unsigned to uint8_t

Amir marked an inline comment as done.Feb 22 2022, 1:39 PM
yota9 accepted this revision.Feb 22 2022, 1:59 PM

LGTM

This revision is now accepted and ready to land.Feb 22 2022, 1:59 PM
Amir updated this revision to Diff 410716.Feb 22 2022, 10:55 PM

Use addEHInfo to test encodeAnnotationImm without exposing it as public method/function

Amir edited the summary of this revision. (Show Details)Feb 22 2022, 10:56 PM
Amir updated this revision to Diff 410726.Feb 23 2022, 12:14 AM

Fix shared build

Amir updated this revision to Diff 410917.Feb 23 2022, 12:55 PM

Use extractAnnotationValue in the check

maksfb added inline comments.Feb 23 2022, 12:57 PM
bolt/unittests/Core/MCPlusBuilder.cpp
114

Any reason for x86_64-only?

Amir added inline comments.Feb 23 2022, 1:03 PM
bolt/unittests/Core/MCPlusBuilder.cpp
114

createCall is not defined in AArch64MCPlusBuilder

yota9 added inline comments.Feb 23 2022, 1:10 PM
bolt/unittests/Core/MCPlusBuilder.cpp
114

Sorry, but why not just use BC->MIB->addAnnotation directly?

Amir added inline comments.Feb 23 2022, 1:16 PM
bolt/unittests/Core/MCPlusBuilder.cpp
114

There's no way to put a predefined Value into an encoded annotation immediate using addAnnotation.
addAnnotation allocates a MCSimpleAnnotation<ValueType> using placement new, and passes the pointer to allocated object to
setAnnotationOpValue.

https://github.com/llvm/llvm-project/blob/main/bolt/include/bolt/Core/MCPlusBuilder.h#L1639

yota9 accepted this revision.Feb 23 2022, 1:19 PM
yota9 added inline comments.
bolt/unittests/Core/MCPlusBuilder.cpp
114

Oh yes, I forgot about that, thanks.

maksfb added inline comments.Feb 23 2022, 2:04 PM
bolt/unittests/Core/MCPlusBuilder.cpp
114

createCall is not defined in AArch64MCPlusBuilder

Interesting. But it has `createTailCall(). Can we use that for testing?

Amir updated this revision to Diff 410939.Feb 23 2022, 2:10 PM

Use createTailCall

Amir marked 4 inline comments as done.Feb 23 2022, 2:11 PM
maksfb accepted this revision.Feb 23 2022, 2:12 PM

LGTM. Thanks for the fix.

yota9 added inline comments.Feb 23 2022, 2:13 PM
bolt/unittests/Core/MCPlusBuilder.cpp
113–131

Still under X86 :)

Amir added inline comments.Feb 23 2022, 2:19 PM
bolt/unittests/Core/MCPlusBuilder.cpp
113–131

Good catch!

Amir updated this revision to Diff 410941.Feb 23 2022, 2:20 PM

Move out from X86_AVAILABLE

Amir marked an inline comment as done.Feb 23 2022, 2:21 PM
yota9 accepted this revision.Feb 23 2022, 2:21 PM

Thank you @Amir!

Amir edited the summary of this revision. (Show Details)Feb 23 2022, 3:46 PM
This revision was automatically updated to reflect the committed changes.