[PATCH] D54589: [clang][UBSan] Sanitization for alignment assumptions.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:198
+assume-aligned-like attributes), `object-size``, and ``vptr`` checks do not
+apply to pointers to types with the ``volatile`` qualifier
 
----------------
lebedev.ri wrote:
> rjmccall wrote:
> > Is there a reason for this exception?
> Are you asking about the LHS of the diff, or about adding an exception to that for this sanitizer?
> I'm adding an exception here because i don't know what should be done here.
> Does it make sense to emit an assumptions for volatile pointers, but do not sanitize these assumptions?
> Are you asking about the LHS of the diff, or about adding an exception to that for this sanitizer?

I'm asking about adding a new exception for one portion of one sanitizer.

> I'm adding an exception here because i don't know what should be done here.

Okay, that's not a good enough reason.

The overall rule for annotation-based language/tool designs is that explicit/specific/close wins over implicit/general/distant.  So the question is: how does that rule apply here?

You can't end up with a pointer to `volatile` completely implicitly — at some point, a programmer was explicit about requesting `volatile` semantics, and that has somehow propagated to this particular access/assumption site.  So that's a pretty strong piece of information, and if we have a general rule for the sanitizers that `volatile` bypasses the check, it's generally a good idea to be consistent with that.

On the other hand, these assumption annotations are themselves always explicit, right?  If you have to be explicit about putting `align_value` on a specific pointer variable, and that pointer just happens to be `volatile`-qualified, we probably *shouldn't* bypass the check: that's about an explicit, specific, and close as a programmer can get, just short of literally writing it on every access to the variable.  The only counter-argument is that maybe the pointer is only `volatile`-qualified because of template instantiation or something.

So I think it makes sense to enforce it for at least some of these annotations and/or builtin calls, but we should be clear about *why* it makes sense.  However, it's possible that I may be misunderstanding part of the motivation behind the general exception for `volatile`, so you should reach out for input from the UBSan etc. people.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2467
+    llvm::Value *OffsetValue, llvm::Value *TheCheck,
+    llvm::Instruction *Assumption) {
+  assert(Assumption && isa<llvm::CallInst>(Assumption) &&
----------------
lebedev.ri wrote:
> rjmccall wrote:
> > What's the deal with the two different source locations?
> The first one points to the source-location of this alignment assumption.
> The second one *may* point to the location where the alignment was specified.
> See e.g. "test/ubsan/TestCases/Pointer/alignment-assumption-attribute-align_value-on-lvalue.cpp" in https://reviews.llvm.org/D54590#change-jI44M13yrBNo
I see, so the runtime diagnostic can point to the second location if it's known.

The parameter names do not make that clear at all.  It should at least be documented, and you should probably change the variable names to be clearer.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54589