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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: docs/ReleaseNotes.rst:310
+      char **passthrough(__attribute__((align_value(1024))) char **x) {
+        return x; // <- check the
+      }
----------------
unfinished


================
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:
> lebedev.ri wrote:
> > rjmccall wrote:
> > > 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.
> > Tried with nullability attributes https://godbolt.org/z/rJUb9U
> > They do not bypass this "ignore volatile".
> > So i guess i will drop this exception.
> Whoops, i meant `nullable` of course, showed the wrong snippet https://godbolt.org/z/DbzKK0
Okay, WFM.


================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:1476
         llvm::Value *PtrValue = CGF.EmitScalarExpr(E);
-        CGF.EmitAlignmentAssumption(PtrValue, Alignment);
+        CGF.EmitAlignmentAssumption(PtrValue, E, {/*No second loc needed*/},
+                                    Alignment);
----------------
Same comment here about `SourceLocation()`.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2508
+// Loc - where the diagnostic will point, where in the source code this
+//  alignemnt has failed.
+// SecondaryLoc - if present (will be present if sufficiently different from
----------------
typo


Repository:
  rC Clang

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

https://reviews.llvm.org/D54589