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

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
> >
> >

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/20151109/53446d86/attachment.html>

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.

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/20151109/32a407d2/attachment.html>

> On Nov 9, 2015, at 2:07 PM, Reid Kleckner via llvm-dev <llvm-dev at
lists.llvm.org> 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.

During the BoF at the LLVM Dev meeting, if I understood correctly (I hope other
can confirm, Eric?), some people asked about this issue and it was said that
3.7.1 will take the fix.

— 
Mehdi


> 
> On Mon, Nov 9, 2015 at 9:29 AM, Tom Stellard <tom at stellard.net
<mailto: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
<mailto:sabre at nondot.org>> wrote:
> >
> > >
> > > > On Oct 27, 2015, at 8:10 AM, Tom Stellard <tom at
stellard.net <mailto:tom at stellard.net>> wrote:
> > > >
> > > > Hi Chris,
> > > >
> > > > I would like to get your opinion on merging r242372
> > > > (http://reviews.llvm.org/rL242372
<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
> > >
> > >
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20151109/7302548f/attachment.html>