Page MenuHomePhabricator

[clang][CodeGen] add fn_ret_thunk_extern to synthetic fns
ClosedPublic

Authored by nickdesaulniers on Jul 13 2022, 5:30 PM.

Details

Summary

Follow up fix to
commit 2240d72f15f3 ("[X86] initial -mfunction-return=thunk-extern
support")
https://reviews.llvm.org/D129572

@nathanchance reported that -mfunction-return=thunk-extern was failing
to annotate the asan and tsan contructors.
https://lore.kernel.org/llvm/Ys7pLq+tQk5xEa%2FB@dev-arch.thelio-3990X/

I then noticed the same occurring for gcov synthetic functions.

Similar to
commit 2786e67 ("[IR][sanitizer] Add module flag "frame-pointer" and set
it for cc1 -mframe-pointer={non-leaf,all}")
define a new module level MetaData, "fn_ret_thunk_extern", then when set
adds the fn_ret_thunk_extern IR Fn Attr to synthetically created
Functions.

Fixes https://github.com/llvm/llvm-project/issues/56514

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 5:30 PM
nickdesaulniers requested review of this revision.Jul 13 2022, 5:30 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 13 2022, 5:30 PM
nickdesaulniers added inline comments.
clang/test/CodeGen/attr-function-return.c
108

https://lore.kernel.org/llvm/Ys7pLq+tQk5xEa%2FB@dev-arch.thelio-3990X/ also mentions asan.module_dtor(). I wonder what flags produce that?

This cleans up the objtool warnings I initially reported and does not introduce any other regressions in my Linux kernel build and QEMU boot tests.

MaskRay accepted this revision.Jul 14 2022, 10:40 AM

https://github.com/llvm/llvm-project/issues/56514

Fixes #56514

Just use Fixes https://github.com/llvm/llvm-project/issues/56514 or Fixes #56514. GitHub can close the issue automatically.

clang/test/CodeGen/attr-function-return.c
9

I feel that having 3 RUN lines is excessive. Since we already have a test that asan/tsan/gcov respect getModuleFlag, it is not necessary for a new one to duplicate all the combinations.

llvm/docs/LangRef.rst
7372

keep the list sorted

This revision is now accepted and ready to land.Jul 14 2022, 10:40 AM
MaskRay added inline comments.Jul 14 2022, 10:42 AM
llvm/lib/IR/Function.cpp
357

How about "function_return_thunk_extern"? fn doesn't abbreviate much. Having the function_return substring makes it easy for grepping.

nickdesaulniers marked 2 inline comments as done.
  • rename md idententifier, sort langref

Thanks for the review!

clang/test/CodeGen/attr-function-return.c
9

All three have caused problems for us; I'd prefer to retain each.

llvm/lib/IR/Function.cpp
357

Done, but note that this was done to match the Function Attribute.

nickdesaulniers edited the summary of this revision. (Show Details)Jul 14 2022, 11:23 AM
  • resort langref, oops
MaskRay accepted this revision.Jul 14 2022, 11:31 AM
MaskRay added inline comments.
clang/test/CodeGen/attr-function-return.c
9

Having asan/tsan may invite others to add more sanitizer tests to this not-so-related file. I can step back and think that gcov/asan are sufficient.

This revision was landed with ongoing or failed builds.Jul 14 2022, 11:31 AM
This revision was automatically updated to reflect the committed changes.
clang/test/CodeGen/attr-function-return.c
9

If such sanitizers produce synthetic functions, we should add them to this file.

MaskRay added inline comments.Jul 14 2022, 11:37 AM
clang/test/CodeGen/attr-function-return.c
9

No. The sanitizers should pick a dedicated test file and be added there, if the use case is important enough.

Today we have -mfunction-return=thunk-extern, tomorrow we can have -mfoobar. We should not add {-mfunction-return=thunk-extern,-mfoobar} * {-fsanitize=address,-fsanitize=thread,...} for every combination.