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