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

Fri Feb 26 20:04:44 PST 2016

On Fri, Feb 26, 2016 at 7:59 PM Hal Finkel <hfinkel at anl.gov> wrote:

> *From: *"Chandler Carruth" <chandlerc at google.com>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org>, "Philip Reames" <
> listmail at philipreames.com>, "Duncan P. N. Exon Smith" <
> dexonsmith at apple.com>, "Xinliang David Li" <xinliangli at gmail.com>,
> "Sanjoy Das" <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> wrote:
>
>> *From: *"Chandler Carruth" <chandlerc at google.com>
>> *To: *"Hal Finkel" <hfinkel at anl.gov>
>>
>> *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org>, "Philip Reames" <
>> listmail at philipreames.com>, "Duncan P. N. Exon Smith" <
>> dexonsmith at apple.com>, "Xinliang David Li" <xinliangli at gmail.com>,
>> "Sanjoy Das" <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> wrote:
>>
>>
>>> > From: "Chandler Carruth" <chandlerc at google.com>
>>> > To: "Hal Finkel" <hfinkel at anl.gov>, "Sanjoy Das" <
>>> sanjoy at playingwithpointers.com>
>>> > Cc: "llvm-dev" <llvm-dev at lists.llvm.org>, "Philip Reames" <
>>> listmail at philipreames.com>, "Duncan P. N. Exon Smith"
>>> > <dexonsmith at apple.com>, "Xinliang David Li" <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.

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.

I'm fine if you want an option to enable unsafe behavior here, but I think
we have to have an option that provides correct results and I frankly think
it should be the default.

>
> Furthermore, you agree that there is a technical solution that satisfies
> these requirements (placing functions into their own hashed comdat
> sections),
>

No, I think that is a significant overstatement. I think that this would
require a pretty sizable amount of work to get the size regression to be
tolerable. I don't know how much work, nor when it could be done.

> and I don't see why this is not relatively-straightforward to implement,
> and so if we want to fix this bug, we should implement it. In the common
> case (where everything is optimized at the same level), I don't see why it
> has any additional overhead. We should be able to privatize aggressively
> under this scheme.
>

I don't have any confidence in our ability to perfectly merge privatized
routines. Maybe you do, but I genuinely do not how to do it with sufficient
reliability today.

I have ideas about how to get a *good* approximation that I think will
recover much of the penalty. But I think it would still need a size
threshold....

>
>
>  -Hal
>
>
> I think we should pretty immediately:
>
> 1) Make our IPO passes conservatively correc
> 2) Leave comments about how to add privatization, but explain the code
> size cost incurred by it
>
> I have no idea if anyone is even working on privatization (I'm not) or
> function merging (no ETA at all, its really far down my queue). I think we
> should decouple all these pieces. Once things are correct, folks can add a
> very size-conservative privatization transformation to IPO routines if we
> don't have merging, or a fairly aggressive one if we do have merging. And
> if we add merging later we can re-tune the privatization.
>
> -Chandler
>
>
>>
>> Thanks again,
>> Hal
>>
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160227/021496db/attachment.html>