[PATCH] D54590: [compiler-rt][UBSan] Sanitization for alignment assumptions.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: test/ubsan/TestCases/Pointer/alignment-assumption-attribute-align_value-on-lvalue.cpp:23
+  struct ac_struct x;
+  x.a = argv; // FIXME: it is weird that this does not also have an assumption.
+  load_from_ac_struct(&x);
----------------
rjmccall wrote:
> lebedev.ri wrote:
> > morehouse wrote:
> > > Any idea why the assignment doesn't trigger a report while the unused return value does?
> > Because no alignment assumption is being emitted in that case.
> > 
> > The question is, given such a cast that increases the alignment,
> > if the run-time check is to fail, is the cast is itself UB already,
> > or is it UB to use the resulting mis-aligned pointer?
> > (CC @rjmccall, @rsmith)
> > 
> > Regardless, this is mainly a question for a clang side,
> > and i think it can be addressed as a follow-up.
> > 
> The C standard says the cast is UB; q.v. C99 6.3.2.3p6:
> 
> > A pointer to an object or incomplete type may be converted to a pointer to a different object or incomplete type. If the resulting pointer is not correctly aligned57) for the pointed-to type, the behavior is undefined.
> 
> The C++ standard says the cast has an unspecified result; q.v. C++ N4640 [expr.static.cast]p13:
> 
> > A prvalue of type “pointer to cv1 void” can be converted to a prvalue of type “pointer to cv2 T”, where T is an object type and cv2 is the same cv-qualification as, or greater cv-qualification than, cv1. If the original pointer value represents the address A of a byte in memory and A does not satisfy the alignment requirement of T, then the resulting pointer value is unspecified.
> 
> Actually using that unspecified value would then be UB.
> 
> If UBSan enforces alignment at the point of access, that's a significantly weaker check than either standard would allow.  The C++ rule is basically the C rule except that the value is allowed to inertly propagate as long as it's not used, which in practice is unlikely — the real difference is that the C rule is extremely easy to enforce and the C++ rule is extremely difficult.  All that said, prudentially, I think the rule that UBSan currently enforces is probably the right balance.
Thank you for chiming in!

I did find that C++ part, and thus didn't really consider this as a bug before.

I suppose we could either emit the check when in C mode;
or be pedantic, and specify that in C++ case a run-time diag **should** happen,
but indeed, i'm not sure what is best here.
Early detection would be better if you ask me though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54590