[llvm-dev] Request to merge r242372 into the 3.7 branch: Fix for C API incompatibility between 3.6 and 3.7.

Tue Nov 10 10:06:23 PST 2015

This is the conclusion of everyone else as well, yes.

Please go ahead Tom, and thanks very much.

-eric

On Tue, Nov 10, 2015 at 9:51 AM deadal nix <deadalnix at gmail.com> wrote:

> The thing is, 3.7 broke ABI not 3.7.1 . 3.7.1 is compatible with 3.6.x and
> 3.8.x (or what we know of it so far).
>
> Reducing the window during which this ABI differs is a plus.
> On Nov 10, 2015 09:13, "Tom Stellard" <tom at stellard.net> wrote:
>
>> On Mon, Nov 09, 2015 at 10:22:01PM +0000, Eric Christopher wrote:
>> > We covered this particular patch a bit at the C API BoF at the
>> conference,
>> > and I think the general opinion is that this was just an unintentional
>> bug
>> > and that fixing it is in 3.7.1 is the best solution forward.
>> >
>>
>> Did anyone object to the ABI breakage this would cause between 3.7.0 and
>> 3.7.1?
>> My impression is that most users of the stable releases care a lot about
>> ABI
>> stability, and this change will affect everyone not just users of the C
>> API.
>>
>> We will also need to update the SONAME for 3.7.1 and fix the symlinks to
>> account
>> for this change.
>>
>> If the consensus is that fixing the C API here is worth breaking ABI
>> between
>> 3.7.0 and 3.7.1, that is fine. I just want to make sure people were aware
>> of the downsides of breaking the ABI here.
>>
>> -Tom
>>
>> > I'll send out more on the C API BoF as I get out from under a pile of
>> email.
>> >
>> > -eric
>> >
>> >
>> > On Mon, Nov 9, 2015 at 2:07 PM Reid Kleckner <rnk at google.com> wrote:
>> >
>> > > We knew this was an ABI break. Here was the logic for why we want it
>> in
>> > > the 3.7.1 release:
>> > >
>> > > The C API is supposed to be stable. The fact that the 3.7.0 release
>> > > contained a modified LLVMBuildLandingPad function was a bug. Anyone
>> who
>> > > relies on this API basically cannot use the 3.7.0 release. The set of
>> users
>> > > using the C API and dealing with EH is probably small, so we chose
>> not a to
>> > > rush out a 3.7.1 release with a fix for this. Instead, we let the fix
>> roll
>> > > into 3.7.1, which will now have a backwards compatible C API and ABI
>> with
>> > > pre 3.7.0 LLVM.
>> > >
>> > > ---
>> > >
>> > > Anyway, I don't really care whether this gets merged or not. I am not
>> a
>> > > stakeholder in the stability of the C API. I would really prefer it
>> if the
>> > > C API users voiced an opinion on whether they want the C API to be
>> more
>> > > stable or closer to LLVM's in memory representation.
>> > >
>> > > On Mon, Nov 9, 2015 at 9:29 AM, Tom Stellard <tom at stellard.net>
>> wrote:
>> > >
>> > >> On Wed, Oct 28, 2015 at 05:32:17AM +0000, Eric Christopher wrote:
>> > >> > On Tue, Oct 27, 2015 at 8:15 PM Chris Lattner <sabre at nondot.org>
>> wrote:
>> > >> >
>> > >> > >
>> > >> > > > On Oct 27, 2015, at 8:10 AM, Tom Stellard <tom at stellard.net>
>> wrote:
>> > >> > > >
>> > >> > > > Hi Chris,
>> > >> > > >
>> > >> > > > I would like to get your opinion on merging r242372
>> > >> > > > (http://reviews.llvm.org/rL242372) into the 3.7 branch.
>> > >> > > > The signature of the C API function LLVMBuildLandingPad
>> > >> > > > changed from the 3.6.0 to 3.7.0 release, so the C API
>> > >> > > > is currently incompatible between these to releases.
>> > >> > >
>> > >> > > This is a narrow enough API that it is probably only used by a
>> few
>> > >> > > clients.  I’d be happy for us to do whatever those clients would
>> like
>> > >> to
>> > >> > > see with this.
>> > >> > >
>> > >> >
>> > >> > Normally I'd prefer not to change API in a point release, but I
>> think
>> > >> it's
>> > >> > fine to change it back here to match the previous releases and
>> current
>> > >> > trunk. Just needs something in the release notes.
>> > >> >
>> > >>
>> > >> Adding the correct llvm-dev list this time...
>> > >>
>> > >> I've just realized that this patch changes the ABI and would make
>> 3.7.0
>> > >> and 3.7.1
>> > >> binary incompatible.  I think breaking the stable ABI is worse than
>> > >> breaking the C API,
>> > >> so I would prefer to either drop this patch or come up with a
>> work-around.
>> > >>
>> > >> One possible work-around would be to keep the LLVMBuildLandingPad
>> > >> signature the
>> > >> same and then add this to Core.h:
>> > >>
>> > >> #ifdef DEBUG // Not sure whether we should use this or ifndef NDEBUG
>> > >>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name) \
>> > >>     dbgs() << "Warning: PersnFn parameter ignored.  You must
>> explicitly
>> > >> set the " \
>> > >>               "personality function on the parent function with " \
>> > >>               " LLVMSetPersonalityFn().  This behavior changed in
>> LLVM
>> > >> 3.7"; \
>> > >>     LLVMBuildLandingPad(B, Ty, Name)
>> > >> #else
>> > >>   #define LLVMBuildLandingPad(B, Ty, PersFn, Name)
>> > >>     LLVMBuildLandingPad(B, Ty, Name);
>> > >>
>> > >> I'm open to other suggestions.  What do people think about this?
>> > >>
>> > >> -Tom
>> > >>
>> > >>
>> > >> >
>> > >> >
>> > >> > >
>> > >> > > -Chris
>> > >> > >
>> > >> > > >
>> > >> > > > Merging this commit will make the 3.7.1 C API compatible
>> > >> > > > with 3.6.x and current trunk, but it will make the 3.7.1 C API
>> > >> > > > incompatible with 3.7.0.
>> > >> > > >
>> > >> > > > Not merging this commit will mean the 3.7.x C API will
>> > >> > > > be incompatible with 3.6.x and current trunk, but
>> > >> > > > the C API for 3.7.0 and 3.7.1 will be compatible.
>> > >> > > >
>> > >> > > > It took me a while to wrap my head around this, let me know if
>> you
>> > >> have
>> > >> > > > any questions.
>> > >> > > >
>> > >> > > > -Tom
>> > >> > >
>> > >> > >
>> > >>
>> > >
>> > >
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151110/685b1cc4/attachment.html>