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

[Peter Collingbourne via Phabricator via cfe-commits]

pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

LGTM

> 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 the answer to my question is "yes" can you please update the commit message to not talk about changing the function signature since that's not the problem here.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1843
+
+  auto *FTRTTIProxy = new llvm::GlobalVariable(
+      TheModule, Addr->getType(),
----------------
ychen wrote:
> pcc wrote:
> > ychen wrote:
> > > pcc wrote:
> > > > Are these proxy variables necessary? I think that now that we have custom code generation for this you should be able to use a GOTPCREL relocation to refer to the global.
> > > I attempted the GOTPCREL approach in a local branch. It didn't work for a reason that I couldn't remember off the top of my head. I'll find out.
> > > 
> > > > I think that now that we have custom code generation for this 
> > > 
> > > Sorry, I don't quite follow which `custom code generation` you are referring to. Do you mean the changes in `AsmPrinter.cpp`?
> > > Sorry, I don't quite follow which custom code generation you are referring to. Do you mean the changes in AsmPrinter.cpp?
> > 
> > Yes, that's what I meant.
> @pcc, I looked into my local branch that uses PCREL approach. It is not simpler/cleaner than the current approach due to the X86/X86-64, macho/ELF difference. These need their specific relocation types.
Okay, let's go with this then.


================
Comment at: llvm/docs/LangRef.rst:5209
 know or know it can't preserve. Currently there is an exception for metadata
 attachment to globals for ``!type`` and ``!absolute_symbol`` which can't be
 unconditionally dropped unless the global is itself deleted.
----------------
We should add `!func_sanitize` here as well.


================
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)) {
----------------
ychen wrote:
> pcc wrote:
> > What if we have both prologue data and this metadata? Should it be an error?
> It may or may not be depending on what is in the prologue data (if it just adds up a counter in the prologue, it should be fine?). How about clarifying this in Langref for this new `MD_func_sanitize`? If being conservative, we could error this out in the Verifier. WDYT?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115844