Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

additional tooling for ABI compatibility verification #20654

Open
andrewrk opened this issue Jul 16, 2024 · 10 comments
Open

additional tooling for ABI compatibility verification #20654

andrewrk opened this issue Jul 16, 2024 · 10 comments
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Jul 16, 2024

ABI mismatch is uncheckable undefined behavior, so it's important to get right. Currently, Zig lacks tooling to help identify such problems. In fact, when the canonical ABI definition lives in a C header file, Zig's safety here is inferior to C. A C compiler will provide errors if the .h file does not match the .c file, but Zig cannot provide errors when a .zig file does not match a .h file.

Observation: the missing primitive here is the ability to compare two Zig compilations with respect to their exported and external ABIs.

So let's start by adding such a primitive to zig build-exe and friends:

  -femit-abi[=path]         Output ABI description file (.abi)
  -fno-emit-abi             (default) Do not output ABI description file (.abi)

I use the extension .abi as opposed to e.g. .zabi because it is a general-purpose format that could be embraced by other tooling; it is not Zig-specific.

This file is itself a binary file but would have a corresponding textual representation. When using Zig tooling, textual representation would just render as Zig source code like this:

// ...
pub extern var epoxy_glCreateShader: ?*const fn (i32) callconv(.C) i32;
pub extern fn glfwGetTime() f64;
pub extern fn pread64(c_int, [*]u8, usize, i64) isize;
// ...

Of course it would be trivial to output a different language's bindings as a textual representation instead.

An important feature of this ABI description data is the ability to find out when they are incompatible. I envision this being a subcommand of the Zig compiler such as zig abi-check which lives in lib/compiler/* along with the other lazily-compiled subcommands. A corresponding build system step would make it easy to wire things up in a build script like this:

const abi_check_step = b.addAbiCheck(.{
    .expected = externally_provided_abi_file,
    .candidate = my_zig_app.getOutputAbiFile(); // this will cause -femit-abi to be passed to the compiler
});

This would cause the step to fail if my_zig_app used ABI that mismatched the expected ABI.

Doing this in the build system this way is nice because it gives a chance for third party projects to embrace the .abi standard and participate both on the giving and receiving end of this.

When an ABI check fails, a handy thing to do would be to convert both to textual representation and perform a line diff algorithm on them. This is already implemented I believe by std.testing.expectEqualStrings.

After translating C code to Zig code, it is then possible to produce an ABI file. For example:

const externally_provided_abi_file = b.addObject(.{
    .root_source_file = b.addTranslateC(.{
        .root_source_file = b.path("foo.h"),
    }).getOutput(),
}).getOutputAbiFile();

With this pattern, it is possible to verify that one's Zig code adheres to the ABI codified by a particular .h file, with a particular set of compilation settings.

I imagine a couple more pieces here to complete the puzzle.

One, a subcommand (perhaps part of zig abi-check) and corresponding build step that converts an .abi file to Zig source code:

const zig_source_file = b.addAbiToSource(.{
    .abi_file = b.path("input.abi"),
}).getOutput();

Here, zig_source_file could now be used in a module as auto-generated bindings. These bindings would be preferable to translated C header files because ABI description files would contain richer type information, such as optional pointers, and pointer sizes. Furthermore, ABI description files could be lowered to C header files, although it would lack the ability to output C macros (I would consider that a feature rather than a limitation).

Finally, I imagine embedding these ABI description files into custom linker sections. The idea here is that it would be nice to know the ABI of already-compiled dynamic libraries that reside on one's system. This way you can use them without needing the .h files to be provided, and you don't get the problem that happens sometimes where you compile against one version of header files but the dynamic library on the system is a different version.

Open questions:

  • Does such a standard already exist? Should we jump on a bandwagon instead of inventing something new?
  • Support enums? packed structs? I'm thinking yes.
  • Check for strict equality? Should there be a notion of compatibility? Should there be compatibility-checking settings? I'm thinking that there is indeed a notion of compatibility. For example, if the ABI defines a usize but you want to have an enum on the other side, that is well-defined behavior according to the ABI. However, one might want to tweak what kinds of compatibility are or are not counted as mismatch.
  • Should it allow documentation strings to be attached to symbol names? If so, there could be the notion of a "load-bearing behavior documentation" where any changes to the documentation string means incompatibility. A paranoid developer may want to turn this on, in order to keep their documentation strings up-to-date, or at least notice when the documentation upstream changes.
  • The full specification of the ABI description file format and all its details. For example: include optional parameter names? (probably yes)

Related:

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Jul 16, 2024
@andrewrk andrewrk added this to the 0.15.0 milestone Jul 16, 2024
@nektro
Copy link
Contributor

nektro commented Jul 16, 2024

This file is itself a binary file

I like this idea but think ^ would be a critical mistake.
also this seems very similar to https://en.wikipedia.org/wiki/Interface_description_language

@alexrp
Copy link
Contributor

alexrp commented Jul 16, 2024

I love this idea, but also agree re: binary format being a mistake.

  • It makes pull requests that include ABI changes to a project basically unreviewable unless you trust the author.
  • It causes significant growth in repo size over time (at least for Git).
  • There is immense value in being able to use tools like git blame on the ABI file.

As a real-world example of where this is valuable in text form, I would point to the .NET runtime. A randomly-selected pull request that implements an API proposal: dotnet/runtime#104651

Notice how the included changes to the ref assembly give you a neat overview of the public API changes without even needing to look at the concrete implementation.

@mlugg
Copy link
Member

mlugg commented Jul 16, 2024

You've touched on this already, but the job of -femit-abi would be incredibly similar to the current job of -femit-h. Now, given the whole addAbiToSource idea -- which I am a fan of -- I think it would make sense to embed documentation strings inside the ABI file (so that the generated Zig bindings can have useful doc comments). In that case, the emit-h feature would be emitting files with a strict subset of the information in the equivalent ABI files (due to C's less specific type system), so could be eliminated entirely in favor of generating an ABI description file and having tooling (perhaps provided by the build system, or perhaps an external project) convert this to a header file. That's an idea I'm very fond of, because the same functionality exists (IMO it would be fair to have this conversion directly in std.Build), but the compiler itself doesn't carry forward any "bias" towards using the syntax and type system of C.


@nektro

This file is itself a binary file

I like this idea but think ^ would be a critical mistake.

Please justify assertions like this. Why would it be a mistake? As it is, this statement has absolutely no value.

[@alexrp, that wasn't aimed at you at all, thank you for the discussion points; GitHub just did something odd and didn't show me your comment until I posted this one!]

@The-King-of-Toasters
Copy link
Contributor

While not a standard, the best known attempt I've seen is abi-cafe. APIs are described in KDL to generate C ABI tests for multiple toolchains. You can have clang, gcc, msvc, rustc and zig cc can both export a library and link with each other, with the goal to find where the compilers disagree.

Note that the project isn't about defining an ABI, the author gave up on that a while ago.

@andrewrk andrewrk added the accepted This proposal is planned. label Sep 7, 2024
@andrewrk
Copy link
Member Author

andrewrk commented Sep 7, 2024

I've accepted the proposal. I see there is some pushback on binary format. I still think binary format is the better option, but I'm not saying text is necessarily rejected. If a contributor with a strong vision and sense of taste wants to implement this with a text format, and the text format is nice, then I will accept that counter proposal.

@mlugg
Copy link
Member

mlugg commented Sep 7, 2024

An important thing to consider is how this will harmonize with the upcoming improvements to the CallingConvention enum (to become a union; #21209). We'll want to be able to specify as many callconvs as possible in this ABI file format. We'll also probably want to make it resilient to future additions; so, for instance, if a numeric callconv tag isn't known to an implementation, it can skip over any data associated with that callconv (e.g. regparm data, stack alignment overrides) and just not allow you to use that declaration (e.g. lower it to a @compileError in the generated Zig definitions).

@alexrp
Copy link
Contributor

alexrp commented Sep 7, 2024

One thing I'd like clarification on: How does conditional compilation fit into this?

@mlugg
Copy link
Member

mlugg commented Sep 8, 2024

One thing I'd like clarification on: How does conditional compilation fit into this?

A Zig binary exposing an API already needs to make sure that the namespaces containing exports are referenced to make sure they're included in the binary. So, when generating an ABI file, we just include all referenced exports.

When importing definitions from another ABI file, conditional compilation isn't relevant, aside from that it allows what I mentioned above, where definitions which aren't recognised due to being from future spec versions or something can be lowered to @compileError whilst retaining usability of other definitions.

@alexrp
Copy link
Contributor

alexrp commented Sep 8, 2024

What I mean is, conditional compilation can affect almost every conceivable aspect of the public API/ABI of a Zig library; change a single command line flag and you could, in theory, end up with a completely different API surface.

So is the intent here that users should create multiple ABI files depending on relevant flags?

@mlugg
Copy link
Member

mlugg commented Sep 8, 2024

I mean, sure; the same holds for e.g. C projects, where you can use the preprocessor to include and exclude certain definitions. It doesn't seem particularly controversial that you need the build options to generate accurate API information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

5 participants