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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


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

The PCRel offset is 32bit currently regardless of the code model. The proxy variable might be out of 32-bit offset from the function (for the large model; for the medium model, only if the proxy variable is in `.ldata`,`.lrodata` which is further away), then loading some random data may give false positives. 

We could fix this by making the PCRel offset 64 bit for the medium or the large model. But since no one complains all these years, I guess it is a real edge use case.

> Hopefully we get an error of some sort before hitting this assertion failure, right?

We didn't. I'll add it.



================
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)) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115844