[Webkit-unassigned] [Bug 196533] [META] Undefined behavior bugs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=196533

--- Comment #15 from Filip Pizlo <fpizlo at apple.com> ---
(In reply to Yusuke Suzuki from comment #14)
> (In reply to Filip Pizlo from comment #13)
> > Consider the case of signed integer overflow.  UBSAN will only find the
> > cases where either by folding constants or by the way that the result is
> > used, you have an obvious case of UB.  So it's not like running UBSAN and
> > fixing the bugs it reports will mean that we are avoiding UBs.  We still
> > have them.
> > 
> > On the other hand, -fwrapv completely eliminates that class of UB.
> > 
> > Therefore, if we are concerned about leaving UBs, we should use compiler
> > flags to disable them.
> 
> I think that we use these sanitizers with Debug build with the hope of
> disabling such optimizations.
> Then, we can find such a dangerous constant foldings performed in Release
> build.
> For example, B3 issue was only caused in Release build[1]. I think that
> issue can be caught by Debug+UBSAN effectively.

I see what you mean.  Ignore what I said.  UBSAN will catch all dynamic instances of the issues we care about.

But if that means turning:

    a << b

into:

    a << (b & 31)

in more than 1 or 2 places then we should ask for a compiler flag.

Also, if it means replacing a lot of signed arithmetic with unsigned arithmetic, then same deal: fwrapv is better.

> 
> But, I strongly agree that removing UBs with the flag is more effective to
> remove such bugs than removing random bugs found at runtime.
> 
> [1]: https://bugs.webkit.org/show_bug.cgi?id=153647#c3
> 
> > > > If that is successful, maybe we can just ask for an option in clang to mask
> > > > shift amounts.  I could write an llvm pass to do that in an afternoon.
> > > 
> > > I think this pass should not cause any performance problem in x86.
> > > I searched and learned that UBs in shift amount is because of some CPU
> > > behavior (PowerPC), and having a flag to align the behavior to x86 behavior
> > > does not cause any additional code emission in x86 I think. Maybe, it could
> > > emit masking ops for PowerPC, but we don't care the performance on PowerPC.
> > 
> > Exactly.
> > 
> > > 
> > > Anyway, I think we need a flag for this to explicitly make this type of UBs
> > > well-defined.
> > > But..., we need to add this flag to GCC and VC++ too to remove UBs.
> > 
> > If we only added it to llvm then we would use it there.  Most folks who use
> > this code will use a version compiled by llvm.
> > 
> > Since we're not talking about something that would cause top crashes, I'm
> > not sure we have to worry too much about minority compilers doing the right
> > thing.
> 
> If the UB-related issue occurs in GCC and VC++ due to missing flags, folks
> using these compilers will add some annotation / cast / masking or something
> like that to avoid the issues.
> If it does not matter, I think it is OK.
> 
> But now, I'm a bit optimistic than before. Fortunately, all the supported
> compilers already have a option to remove UBs of signed integer overflow (I
> did not know /d2UndefIntOverflow on VC++).
> I saw many UB-related issues are caused by this UB, and I believe that
> UB-related issues can decrease a lot if we enable this option.

That's cool!

I still think that if llvm had a flag to disable UB and other compilers didn't, then we wouldn't necessarily allow users of other compilers to pollute the code.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190411/e26c7266/attachment.html>