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

[bugzilla-daemon at webkit.org]

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

--- Comment #13 from Filip Pizlo <fpizlo at apple.com> ---
(In reply to Yusuke Suzuki from comment #12)
> (In reply to Filip Pizlo from comment #11)
> > Simple question: has anyone perf tested JSC with -fwrapv?  I believe both
> > clang and gcc support it.
> >
> > If that is perf-neutral, then we won't have to fix any more signed overflow
> > bugs, ever.
> 
> I don't think this option causes performance regression.
> We also need VC++ support too since we build WebKit with VC++ too.
> I've found that undocumented flag `/d2UndefIntOverflow` and I think it
> should work[1].
> Without this option, VC++ leverages UBs of signed integer overflow to
> aggressively optimize the code.
> I agree to enabling this option now to make signed overflow well-defined.
> 
> [1]: https://devblogs.microsoft.com/cppblog/new-code-optimizer/
> 
> > 
> > I'm not sure I understand the argument that says that running a sanitizer
> > and fixing those bugs one-off is superior to just simply adding one compiler
> > flag.
> 
> My main concern is about leaving UBs. If we can avoid UBs by either fixing
> UBs or removing UBs from compiler behavior, I think either is fine.
> Removing UBs can be achieved by either (1) adding a flag to compiler to make
> some UBs well-defined or (2) fixing the spec.
> If we take (1) approach, I think we also need to ensure that all our
> supported compilers have flags to make UBs well-defined.

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.

> 
> > 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.

-- 
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/40863924/attachment.html>