llvm dev - Feb 2016 - Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")

On 02/26/2016 08:04 PM, Chandler Carruth wrote:
> On Fri, Feb 26, 2016 at 7:59 PM Hal Finkel <hfinkel at anl.gov 
> <mailto:hfinkel at anl.gov>> wrote:
>
>         *From: *"Chandler Carruth" <chandlerc at google.com
>         <mailto:chandlerc at google.com>>
>         *To: *"Hal Finkel" <hfinkel at anl.gov
<mailto:hfinkel at anl.gov>>
>         *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org
>         <mailto:llvm-dev at lists.llvm.org>>, "Philip
Reames"
>         <listmail at philipreames.com
>         <mailto:listmail at philipreames.com>>, "Duncan P. N.
Exon Smith"
>         <dexonsmith at apple.com <mailto:dexonsmith at
apple.com>>,
>         "Xinliang David Li" <xinliangli at gmail.com
>         <mailto:xinliangli at gmail.com>>, "Sanjoy Das"
>         <sanjoy at playingwithpointers.com
>         <mailto:sanjoy at playingwithpointers.com>>
>
>         *Sent: *Friday, February 26, 2016 9:41:23 PM
>
>
>         *Subject: *Re: [llvm-dev] Possible soundness issue with
>         available_externally (split from "RFC: Add guard
intrinsics")
>
>         On Fri, Feb 26, 2016 at 7:38 PM Hal Finkel <hfinkel at anl.gov
>         <mailto:hfinkel at anl.gov>> wrote:
>
>                 *From: *"Chandler Carruth" <chandlerc at
google.com
>                 <mailto:chandlerc at google.com>>
>                 *To: *"Hal Finkel" <hfinkel at anl.gov
>                 <mailto:hfinkel at anl.gov>>
>
>                 *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org
>                 <mailto:llvm-dev at lists.llvm.org>>, "Philip
Reames"
>                 <listmail at philipreames.com
>                 <mailto:listmail at philipreames.com>>,
"Duncan P. N.
>                 Exon Smith" <dexonsmith at apple.com
>                 <mailto:dexonsmith at apple.com>>, "Xinliang
David Li"
>                 <xinliangli at gmail.com <mailto:xinliangli at
gmail.com>>,
>                 "Sanjoy Das" <sanjoy at
playingwithpointers.com
>                 <mailto:sanjoy at playingwithpointers.com>>
>                 *Sent: *Friday, February 26, 2016 9:33:55 PM
>
>
>                 *Subject: *Re: [llvm-dev] Possible soundness issue
>                 with available_externally (split from "RFC: Add guard
>                 intrinsics")
>
>                 On Fri, Feb 26, 2016 at 7:26 PM Hal Finkel
>                 <hfinkel at anl.gov <mailto:hfinkel at
anl.gov>> wrote:
>
>
>                     > From: "Chandler Carruth" <chandlerc
at google.com
>                     <mailto:chandlerc at google.com>>
>                     > To: "Hal Finkel" <hfinkel at anl.gov
>                     <mailto:hfinkel at anl.gov>>, "Sanjoy
Das"
>                     <sanjoy at playingwithpointers.com
>                     <mailto:sanjoy at playingwithpointers.com>>
>                     > Cc: "llvm-dev" <llvm-dev at
lists.llvm.org
>                     <mailto:llvm-dev at lists.llvm.org>>,
"Philip Reames"
>                     <listmail at philipreames.com
>                     <mailto:listmail at philipreames.com>>,
"Duncan P. N.
>                     Exon Smith"
>                     > <dexonsmith at apple.com
>                     <mailto:dexonsmith at apple.com>>,
"Xinliang David
>                     Li" <xinliangli at gmail.com
>                     <mailto:xinliangli at gmail.com>>
>                     > Sent: Friday, February 26, 2016 9:01:48 PM
>                     > Subject: Re: [llvm-dev] Possible soundness issue
>                     with available_externally (split from "RFC: Add
>                     guard intrinsics")
>                     >
>                     >
>                     > I think this will have a much higher cost than
>                     my proposal to
>                     > constrain how we deduce function attributes
>                     (which still fixes
>                     > Sanjoy's latest example).
>                     >
>                     > Specifically, I think this will force us to
>                     constrain far too many
>                     > transformations for the sake of code size in
>                     functions that we won't
>                     > inline. Even if we were never going to deduce
>                     function attributes
>                     > for anything in the function (because its big
>                     and reads and writes
>                     > everything), we'll still have to constrain our
>                     transformations just
>                     > because we *might* later deduce a function
>                     attribute that triggers
>                     > these kinds of bugs.
>                     >
>                     > Essentially, you're proposing to limit
>                     intraprocedural optimization
>                     > to when we can successfully to interprocedural
>                     optimization
>                     > ("privatization"), where I'm
suggesting we limit
>                     interprocedural
>                     > optimization to leave intraprocedural
>                     optimization unconstrained.
>                     > Given the ratio of our optimizations (almost all
>                     are intra, very few
>                     > are inter), I'm much more comfortable with the
>                     latter.
>
>                     This is a good point; we can certainly (easily)
>                     delay the privatization decision until we modify
>                     any IPA-level function information (at which point
>                     we can either reject the attribute change (when
>                     optimizing for code size), or keep it locally
>                     (when optimizing for speed). Ideally, you'd want
>                     to delay this even further (until you knew the
>                     attribute information was used), but I'm not sure
>                     that's practical.
>
>                     Actually, what if instead of actually privatizing,
>                     we moved the function into a different comdat
>                     section named after some hash of the function
>                     body? That way, if all versions are actually
>                     optimized identically, we'll still only end up
>                     with one copy in the final executable. If this is
>                     technically possible, it seems like the best kind
>                     of solution.
>
>
>                 This is how I want to do a revamped function merging
>                 anyways and it would fall out naturally of that.
>
>             Excellent, so let's fix this at the same time we implement
>             this function merging (so that we don't have performance
>             regressions in an intermediate state). This will also
>             allow us to have uniform logic across different
>             optimization levels, which is obviously preferable.
>
>
>         I am *extremely* uncomfortable waiting to fix this until
>         merging stuff is in place and we add privatization heuristics
>         to our IPO passes. Those might be years away.
>
>     I'm *extremely* uncomfortable fixing this at all unless it can be
>     done without causing performance regressions. The underlying basic
>     use case (linking together code compiled with different
>     optimization levels), is certainly something I'd like to work
>     properly, but is definitely a far lower priority than optimized
>     code quality and size.
>
>
> Ok, I prioritize this differently: it is absolutely critical. Every 
> binary I have includes files that fit this description. Every single 
> binary.
Chandler, I'm more in agreement with Hal here.  I see your point, but 
given gcc appears to be getting this equally wrong, and we've never seen 
this case in practice, I don't see fixing this as a critical issue.  I'm
also strongly in agreement with Hal that a fix which reverts performance 
for the normal case is unacceptable unless all options have been 
explored and there really are no other choices.
>
> I think that we will discover subtle miscompiles every time we try to 
> make function attributes more powerful until this is fixed. I think 
> this is absolutely critical to fix.
Not sure this is actual true.  In particular, there have been several 
changes to our IPO passes over the last year and I don't remember 
hearing any screaming.  :)

Philip

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160229/ce4a3fa8/attachment.html>

On Mon, Feb 29, 2016 at 2:58 PM, Philip Reames
<listmail at philipreames.com> wrote:
> Chandler, I'm more in agreement with Hal here.  I see your point, but
given
> gcc appears to be getting this equally wrong,
In the bug I filed against GCC, I was told that this is likely fixed
in gcc 6 (I personally haven't verified this)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70018#c5
> and we've never seen this case
> in practice, I don't see fixing this as a critical issue.  I'm also
strongly
Before this weekend I'd have agreed with you (in fact, I said
something on the lines of "may just be a theoretical issue" above),
but the test case to trigger this is surprisingly simple (< 50loc); so
I'm more cautious now.

-- Sanjoy

On Mon, Feb 29, 2016 at 3:24 PM, Sanjoy Das
<sanjoy at playingwithpointers.com> wrote:
> In the bug I filed against GCC, I was told that this is likely fixed
> in gcc 6 (I personally haven't verified this)
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70018#c5
I redact this -- someone confirmed that this issue exists on gcc 6 as
well: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70018#c7

-- Sanjoy