[PATCH] D115844: [ubsan] Using metadata instead of prologue data for function sanitizer

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D115844#3321235 <https://reviews.llvm.org/D115844#3321235>, @ychen wrote:

> In D115844#3321190 <https://reviews.llvm.org/D115844#3321190>, @pcc wrote:
>
>> On the bug you have:
>>
>>   define internal fastcc void @​_Z4callIiE4taskv.resume(%_Z4callIiE4taskv.Frame* noalias nonnull align 8 dereferenceable(24
>>       ) %FramePtr) #​1 prologue <{ i32, i32 }> <{ i32 846595819, i32 trunc (i64 sub (i64 ptrtoint (i8** @​1 to i64), i64 ptrtoint (void ()* @​_Z4callIiE4taskv to i64)) to i32) }> {...}
>>
>> Is it possible for the C/C++ code to take the address of the function `_Z4callIiE4taskv.resume` and call it indirectly?
>
> `*.resume` is a compiler inserted function that is opaque to the programmer. It is called indirectly most of the time if not all the time.

Yes, but not indirectly called from C/C++ but rather from compiler-generated code right? That's presumably why this patch + D116130 <https://reviews.llvm.org/D116130> worked for @zhuhan0.

>> If not, it seems like the right fix would be to arrange for the prologue data to be dropped on the `.resume` function instead of duplicating it there. I would also imagine that whatever signature you have on the `.resume` function would be incorrect since it appears that the coro splitting pass will use a different function signature for that function.
>
> That is addressed by D116130 <https://reviews.llvm.org/D116130>. @rjmccall suggested the direction of this patch (which I agreed) https://reviews.llvm.org/D114728#3159303.

Okay. It seems unfortunate to have to special-case this just because it uses relative relocations. But that's probably the best that we can do without changing the global variable initializer representation (as well as prefix/prologue data) to be blob plus relocations.



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:5171
       (!TargetDecl || !isa<FunctionDecl>(TargetDecl))) {
+    assert((CGM.getCodeGenOpts().CodeModel == "default" ||
+            CGM.getCodeGenOpts().CodeModel == "small") &&
----------------
What happens when building with other code models? Hopefully we get an error of some sort before hitting this assertion failure, right?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:843
+
+  // Emit the function prologue data for the indirect call sanitizer.
+  if (const MDNode *MD = F.getMetadata(LLVMContext::MD_func_sanitize)) {
----------------
What if we have both prologue data and this metadata? Should it be an error?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115844/new/

https://reviews.llvm.org/D115844