This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][UBSan] Add support for handling attributed functions in getUBSanFunctionTypeHash.
ClosedPublic

Authored by usama54321 on Aug 8 2023, 2:50 PM.

Details

Summary

getUBSanFunctionTypeHash does not handle the attributed function case correctly. For a function with an attribute, the QualType passed to the function is a FunctionNoProtoType wrapped in an AttributedType. As a result, the code goes down the !isa<FunctionNoProtoType> branch which is incorrect behavior. This results in an assertion failure inside the call to getFunctionTypeWithExceptionSpec.

The added test is a sanity check that the compiler no longer crashes during compilation.

rdar://113144087

Diff Detail

Event Timeline

usama54321 created this revision.Aug 8 2023, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 2:50 PM
usama54321 requested review of this revision.Aug 8 2023, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 2:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
usama54321 added reviewers: yln, Restricted Project.Aug 8 2023, 2:51 PM
usama54321 edited the summary of this revision. (Show Details)
yln accepted this revision.Aug 8 2023, 4:35 PM
This revision is now accepted and ready to land.Aug 8 2023, 4:35 PM
dcoughlin added a reviewer: rapidsna.
MaskRay added a comment.EditedAug 9 2023, 10:07 AM

The title is very long. Can you shorten it and put additional description to the body of the commit message (aka summary)?

[CodeGen][UBSan] getUBSanFunctionTypeHash does not handle the attributed ...

Did you repeat the subject line in the commit message?

What does the test ubsan-function-attributed.c do? It lacks comments to help reader understand the issue.

Fix commit message and description, and added comment explaining the test.

usama54321 retitled this revision from [CodeGen][UBSan] getUBSanFunctionTypeHash does not handle the attributed function case correctly, leading to an assertion failure. This patch desugars the QualType if it is of AttributedType. to [CodeGen][UBSan] Add support for handling attributed functions in getUBSanFunctionTypeHash..Aug 9 2023, 11:58 AM
usama54321 edited the summary of this revision. (Show Details)
MaskRay added inline comments.Aug 9 2023, 1:30 PM
clang/test/CodeGen/ubsan-function-attributed.c
5

https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-too-little

"A regression test which simply runs the compiler and expects it not to crash has low utilization. It is often better to additionally test some properties of the produced object file."

I think it's better to define another function without __attribute__((ms_abi)), and check that the two functions have the same type hash.

yln added inline comments.Aug 9 2023, 4:37 PM
clang/test/CodeGen/ubsan-function-attributed.c
5

I agree with this and particularly this sounds like a good approach:

I think it's better to define another function without attribute((ms_abi)), and check that the two functions have the same type hash.

rapidsna requested changes to this revision.Aug 10 2023, 11:04 AM

We should add a test to exercise when Ty is wrapped by other sugar types. Could you try with typedef?

clang/lib/CodeGen/CodeGenFunction.cpp
573–575
575

Checking isa<T> still doesn't handle cases where Ty is wrapped in other sugar types (not just AttributedType).

Instead of adding if (isa<AttributedType>(Ty)) above, I would use
Ty->isFunctionNoProtoType() here.

isFunctionNoProtoType is a helper function that uses getAs<FunctionNoProtoType>(). getAs<T> removes any existing sugar until it reaches T or a non-sugar type.

This revision now requires changes to proceed.Aug 10 2023, 11:04 AM
usama54321 retitled this revision from [CodeGen][UBSan] Add support for handling attributed functions in getUBSanFunctionTypeHash. to [CodeGen][UBSan] Add support for handling attributed functions in getUBSanFunctionTypeHash..

Updated PR according to feedback

Updated commit message

rapidsna accepted this revision.Aug 15 2023, 2:21 PM

The changes look good to me. Ideally, we could add tests with multiple attributes and possibly with other sugar types.

This revision is now accepted and ready to land.Aug 15 2023, 2:21 PM
MaskRay accepted this revision.Aug 15 2023, 3:07 PM
MaskRay added inline comments.
clang/test/CodeGen/ubsan-function-attributed.c
4

It's useful to add // CHECK-LABEL: f: interleaving with .long xxx. The FileCheck error will be better when the hash changes.

This revision was automatically updated to reflect the committed changes.