[llvm] [RemoveDIs] Add a 'BeforeDbgRecords' parameter to C API isnt insertion functions (PR #92417)

[Nikita Popov via llvm-commits]

nikic wrote:

> Thanks for taking a look @nikic
> 
> > It is not possible to change the signature of a function in the C API. You can only add a new function with the new signature.
> 
> Oh, I can't see this rule in the [developer policy](https://llvm.org/docs/DeveloperPolicy.html#c-api-changes). But that would make the changes more stable.

The phrasing here as a bit unclear as to what "best effort stability" means, but the rules are basically:

 * You are allowed to add new functions.
 * You shouldn't remove functions, but are allowed to do so if there's a strong reason (e.g. the underlying feature has been removed from LLVM).
 * You are never allowed to make an ABI-breaking change to an existing function.

Changes to the C API should, at worst, produce a linker error, but never introduce an FFI mismatch.

> > The usual pattern would be to add LLVMPositionBuilder2 (in this case an alternative would be to add a new function with the parameter set to the non-default value).
> 
> The reason we need to modify the API is that at the moment it's possible to insert PHIs after debug records which is illegal and leads to an assertion (edit: more specifically, there's no way to _avoid_ that behaviour using the C API with the new debug info format - inserting a phi before an instruction with debug records attached will always cause the assertion without this patch). I chose to modify existing functions because I figured would help reduce nasty surprises, but if stability is preferable this isn't the right tactic.

I think this does not affect typical use of the C API (where a frontend emits instructions in order -- it will not go back to a previous position), so I wouldn't be overly concerned about accidental misuse.
 
> We could add new functions with the extra parameter, keep the existing functions untouched and forward args and a "default" value for the new parameter to the new functions (possibly this is what you're suggesting already)? I guess this would be most "stable" (and I think then we can omit most of the OCaml changes then).

Yes, that's what I would suggest.

> I wonder if some of the other changes we made need re-reviewing in this case: #84915 - adds some new functions (I imagine this patch is fine)

This is fine.

> #85657 - changes return type of existing functions (is this one ok?)

Yeah, this is not allowed :(

I think what I'd do for the C API is to keep just the new functions you added for dbg records and remove the old functions for intrinsics. Keeping the old name around and making it create intrinsics would be too much of a footgun. And I don't think that we actually need support for the old format in the C API.

https://github.com/llvm/llvm-project/pull/92417