llvm dev - Dec 2014 - [LLVMdev] Using the unused "version" field in the bitcode wrapper (redux)

Where is the testing that proves all this works?  Patches welcome…

Now, I've known QA people who break software in two minutes as naturally as
breathing; I do not have that skill.  Given that I came up with *anything* in
two minutes, I am not filled with confidence.

Right now I'm in the middle of un-borking my bots in the wake of the
upstream "improvements" to GetSVN.cmake, but I will see what else I
can do.
--paulr

From: Sean Silva [mailto:chisophugis at gmail.com]
Sent: Tuesday, November 25, 2014 2:38 PM
To: Robinson, Paul
Cc: Yung, Douglas; llvmdev at cs.uiuc.edu; Reid Kleckner; Bruce Hoult
Subject: RE: [LLVMdev] Using the unused "version" field in the bitcode
wrapper (redux)


On Nov 25, 2014 11:59 AM, "Robinson, Paul" <Paul_Robinson at
playstation.sony.com<mailto:Paul_Robinson at playstation.sony.com>>
wrote:
>
> The fundamental problem is that IR has never been treated as "user
input" before.  It has always been an ephemeral format, and if some
component comes along and sees something it doesn't recognize, that is ipso
facto a compiler bug, not a user input error, and it's perfectly okay to
crash on a compiler bug.  Changing that fundamental assumption would be pretty
pervasive.
Sorry, but this is at variance to my experience. In my experience, the
fundamental assumption is that we should never crash on bad user input. And LLVM
as a library definitely considers bitcode as "user input".

Of course, -disable-verify and similar voids that warranty.
>
>
>
> I will say that the bitcode reader itself is pretty good about identifying
bitcode it doesn't recognize; it's the components deeper inside LLVM
that are more worrisome.
Could you give a couple examples? It sounds like you are talking about a
systemic problem, but so far only one example has been presented.

-- Sean Silva
>
> --paulr
>
>
>
> From: Reid Kleckner [mailto:rnk at google.com<mailto:rnk at
google.com>]
> Sent: Tuesday, November 25, 2014 11:43 AM
> To: Sean Silva
> Cc: Robinson, Paul; Bruce Hoult; llvmdev at cs.uiuc.edu<mailto:llvmdev
at cs.uiuc.edu>; Yung, Douglas
>
> Subject: Re: [LLVMdev] Using the unused "version" field in the
bitcode wrapper (redux)
>
>
>
> On Mon, Nov 24, 2014 at 9:41 PM, Sean Silva <chisophugis at
gmail.com<mailto:chisophugis at gmail.com>> wrote:
>>
>> I fail to see why all that re-engineering effort IS
"required" to support the use-cases we've provided, and why
future-proofing a large body of software that quite honestly was never designed
for it and has genuinely very few reasons why it should be.
>>
>>
>>
>> If we're going to support bitcode as a long-term storage format,
everything that pulls information out of bitcode MUST be future-proofed.  Doing
it piece by piece is a LOT of engineering effort to make LLVM user-friendly in
this way; it was never intended to have that degree of self-defense.
>
>
>
> I don't understand what you mean by "future-proofed" in this
context. If you mean "never crash on bad user input", then your point
doesn't make sense to me. LLVM is engineered to never crash on bad user
input in the same sense that it is engineered to correctly compile code; neither
is allowed, and if either happens it is a bug.
>
>
>
> This is true, but we can say from experience that there are a lot of these
kinds of bugs and it will take a lot of effort to fix them all. Personally, I
think this worth doing. We should be using the new diagnostic handler on the
LLVMContext, and most of LLVM should have a way of returning without exploding.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20141125/b22d13e9/attachment.html>

> Now, I've known QA people who break software in two minutes as
naturally as
> breathing; I do not have that skill.  Given that I came up with *anything*
> in two minutes, I am not filled with confidence.
We don't have good testing. I agree. As with anything in open source,
it is just because people had higher priority stuff. I need to fix LTO
scalability, you were hit by a build system change, etc.

If this is top priority for some group, what that group could do that
is aligned with where we want llvm to go is to start adding tests and
fixing bugs. Fuzzing would be awesome, but even before that, we have
open bugs on llvm.org/bugs about the bitcode reader crashing that have
not been fixed because people had more important stuff to work on.

But note that nothing about the past history of the individual
development priorities changes the direction we want to go: We want to
support reading old bitcode format under the limitations described in
http://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility.
Given the structure of the bitcode, so far it has been possible to do
so for every change but one without adding a version field.

And we *have* a version field. If the mentioned above testing finds a
case that needs a version update, we just need bump
MODULE_CODE_VERSION again.

And we should most certainly not be making the wrapper mandatory or
adding feature that apply only to wrapped bitcode files.

Cheers,
Rafael

> -----Original Message-----
> From: Rafael Espíndola [mailto:rafael.espindola at gmail.com]
> Sent: Monday, December 22, 2014 8:12 AM
> To: Robinson, Paul
> Cc: Sean Silva; Bruce Hoult; Yung, Douglas; llvmdev at cs.uiuc.edu
> Subject: Re: [LLVMdev] Using the unused "version" field in the
bitcode
> wrapper (redux)
> 
> > Now, I've known QA people who break software in two minutes as
naturally
> as
> > breathing; I do not have that skill.  Given that I came up with
> *anything*
> > in two minutes, I am not filled with confidence.
> 
> We don't have good testing. I agree. As with anything in open source,
> it is just because people had higher priority stuff. I need to fix LTO
> scalability, you were hit by a build system change, etc.
> 
> If this is top priority for some group, what that group could do that
> is aligned with where we want llvm to go is to start adding tests and
> fixing bugs. Fuzzing would be awesome, but even before that, we have
> open bugs on llvm.org/bugs about the bitcode reader crashing that have
> not been fixed because people had more important stuff to work on.
> 
> But note that nothing about the past history of the individual
> development priorities changes the direction we want to go: We want to
> support reading old bitcode format under the limitations described in
> http://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility.
> Given the structure of the bitcode, so far it has been possible to do
> so for every change but one without adding a version field.
> 
> And we *have* a version field. If the mentioned above testing finds a
> case that needs a version update, we just need bump
> MODULE_CODE_VERSION again.

I accept that the correct long-term direction is to make the compiler
more robust in this area.  I have, buried on my own to-do list, some
ideas for ways to improve the testing.

It is also the case that *today* the compiler is insufficiently robust
and therefore Sony must use the wrapper as a stop-gap measure to guard 
against those problems until the situation improves (either through our 
efforts or somebody else's).
> And we should most certainly not be making the wrapper mandatory or
> adding feature that apply only to wrapped bitcode files.
I don't think we were suggesting that... certainly the most recent 
proposal made it target-dependent and defaulted to no wrapper, which is
far from "mandatory" and doesn't lend itself to wrapped-only
features.

In the meantime it's yet another private change.
--paulr
> 
> Cheers,
> Rafael