[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6
+  volatile unsigned _v = (val);    \
+  volatile unsigned _a = (amount); \
+  unsigned res = _v << _a;         \
----------------
vsk wrote:
> jfb wrote:
> > vsk wrote:
> > > Does the test not work without the volatiles?
> > It seems that LLVM sees too much without volatile, yes.
> The optimizer isn't running. Perhaps this is necessary because clang's constant folder kicks in?
Probably, I haven't investigate. It's brittle is the main thing, and this forces it to not be.


================
Comment at: llvm/docs/ReleaseNotes.rst:151
+
+One can silence the check with masking, for example:
+
----------------
vsk wrote:
> This could use some more explanation, maybe s/with masking/by clearing bits that would be shifted out/?
I just dropped it, I don't think release notes are the right place for this anyways.


================
Comment at: llvm/docs/ReleaseNotes.rst:153
+
+  `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+
----------------
lebedev.ri wrote:
> vsk wrote:
> > jfb wrote:
> > > lebedev.ri wrote:
> > > > Surely not `~1U`.
> > > Indeed, fixed.
> > I don't think this pattern works when rhs = 0 (https://godbolt.org/z/rvEGqh).
> Surely not `1U` either :)
Right, I don't think it's something we want to explain here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000