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

[usama54321]

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

[MaskRay]

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.

[usama54321]

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

[MaskRay]

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]

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]

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

578

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.

[usama54321]

Updated PR according to feedback

[usama54321]

Updated commit message

[rapidsna]

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

[MaskRay]

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.

clang/lib/CodeGen/CodeGenFunction.cpp

bool CodeGenFunction::AlwaysEmitXRayTypedEvents() const {
   return CGM.getCodeGenOpts().XRayInstrumentFunctions &&
          (CGM.getCodeGenOpts().XRayAlwaysEmitTypedEvents ||
           CGM.getCodeGenOpts().XRayInstrumentationBundle.Mask ==
               XRayInstrKind::Typed);
 }
 
 llvm::ConstantInt *
 CodeGenFunction::getUBSanFunctionTypeHash(QualType Ty) const {
+  if (isa<AttributedType>(Ty))
+    Ty = Ty.getDesugaredType(getContext());
+
   // Remove any (C++17) exception specifications, to allow calling e.g. a
   // noexcept function through a non-noexcept pointer.
   if (!isa<FunctionNoProtoType>(Ty))
     Ty = getContext().getFunctionTypeWithExceptionSpec(Ty, EST_None);
   std::string Mangled;
   llvm::raw_string_ostream Out(Mangled);
   CGM.getCXXABI().getMangleContext().mangleTypeName(Ty, Out, false);
   return llvm::ConstantInt::get(
       CGM.Int32Ty, static_cast<uint32_t>(llvm::xxh3_64bits(Mangled)));
 }
 

clang/test/CodeGen/ubsan-function-attributed.c

+// RUN: %clang_cc1 -emit-llvm -triple x86_64 -std=c17 -fsanitize=function %s -o /dev/null
+
+long __attribute__((ms_abi)) f() {}