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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
