Philip Reames via llvm-dev
2016-Feb-29 22:58 UTC
[llvm-dev] 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>
Sanjoy Das via llvm-dev
2016-Feb-29 23:24 UTC
[llvm-dev] Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")
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 stronglyBefore 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
Sanjoy Das via llvm-dev
2016-Mar-01 18:06 UTC
[llvm-dev] Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")
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#c5I 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