Request to merge r242372 into the 3.7 branch: Fix for C API incompatibility between 3.6 and 3.7.

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

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.

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

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.

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

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.

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

Please go ahead Tom, and thanks very much.

-eric