[BUG] sysroot/usr/include/amidi/AMidi.h is not valid C

[jfgoog]

Description

~/Android/Sdk/ndk/25.0.8775105/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android32-clang ~/Android/Sdk/ndk/25.0.8775105/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/amidi/AMidi.h
/usr/local/google/home/jamesfarrell/Android/Sdk/ndk/25.0.8775105/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/amidi/AMidi.h:230:1: error: must use 'enum' tag to refer to type 'AMidiDevice_Protocol'
AMidiDevice_Protocol AMIDI_API AMidiDevice_getDefaultProtocol(const AMidiDevice *device)
^
enum
1 error generated.

Offending code is: https://android.googlesource.com/platform/prebuilts/ndk/+blame/refs/heads/dev/platform/sysroot/usr/include/amidi/AMidi.h

Needs a typedef:

enum AMidiDevice_Protocol : int32_t { ... };

Or an enum here:

AMidiDevice_Protocol AMIDI_API AMidiDevice_getDefaultProtocol(const AMidiDevice *device)
        __INTRODUCED_IN(33);

Affected versions

r25

Canary version

No response

Host OS

Linux, Mac, Windows

Host OS version

All

Affected ABIs

armeabi-v7a, arm64-v8a, x86, x86_64

Build system

Other (specify below)

Other build system

All

minSdkVersion

API was introduced in 33.

Device API level

All

[rprichard]

enum AMidiDevice_Protocol : int32_t { ... };

The enum underlying type also isn't standard C, but Clang accepts it without complaint. GCC rejects it, but we don't support GCC. Clang can diagnose this usage:

A/test.h:1:8: warning: enumeration types with a fixed underlying type are a Clang extension [-Wfixed-enum-extension]
enum E : int { A, B };
       ^~~~~

AFAIK Clang won't complain about a system header (-isystem or --sysroot), though, so it shouldn't affect any NDK users.

I think we're OK with this Clang dependency?

I think this particular fixed type in AMidi.h is redundant, because the enum type would already be treated like it had a fixed type of 32-bit signed int. Not 100% sure about that though.

[DanAlbert]

I think we're OK with this Clang dependency?

Yes, this was an intentional change made by API council. You get much better diagnostics, IDE behavior, ABI tracking, etc if the APIs can use the enums directly rather than using int32_t everywhere for ABI stability, which we previously required to avoid accidental ABI breaks if the enum's (unspecified) backing type changed. Since clang lets us specify the backing type in C, we can use the enums directly now.

[MarijnS95]

When I originally reported this problem to @jfgoog I was using bindgen in rust-mobile/ndk#324, which is backed by libclang.

[DanAlbert]

Looks like our guidelines were missing the "you need to use a typedef because C" for enums, though we had it for structs. I've sent a CL to fix the copy pasta landmines in the docs, and also the CL fixing this incorrect case.

[philburk]

Is this enum and typedef code in AAudio.h OK?

https://cs.android.com/android/platform/superproject/+/master:frameworks/av/media/libaaudio/include/aaudio/AAudio.h;l=283;drc=d55acea87533759a27e59dec09af8cfe9e9e5e51

[DanAlbert]

It's an older code, but it checks out.

The change to the API guidelines to prefer use of the enum itself with a specified backing type is quite new, and aaudio predates that. aaudio.h will work just fine with C code. You could update the header to do:

typedef enum aaudio_sharing_mode_t : int32_t {
    ...
} aaudio_sharing_mode_t;

The new form has a lot of UX improvements. Most IDE features will work better if the function decls refer to the actual enum. It also will benefit the new ABI tracking tooling that we haven't deployed yet.

We're not pushing anyone toward making those updates (yet), but feel free :)

[MarijnS95]

@philburk It's valid, but as @DanAlbert mentions these anonymous enums are a pain to deal with. The improved UX of a typed enum means we can type it in Rust as well, instead of currently having this untyped "mess":

pub const AAUDIO_SHARING_MODE_EXCLUSIVE: ::std::os::raw::c_uint = 0;
pub const AAUDIO_SHARING_MODE_SHARED: ::std::os::raw::c_uint = 1;
pub type _bindgen_ty_50 = ::std::os::raw::c_uint;
pub type aaudio_sharing_mode_t = i32;

For example, for AMidiDevice_Protocol we're currently generating the (rather verbose...):

impl AMidiDevice_Protocol {
    pub const AMIDI_DEVICE_PROTOCOL_UMP_USE_MIDI_CI: AMidiDevice_Protocol = AMidiDevice_Protocol(0);
}
impl AMidiDevice_Protocol {
    pub const AMIDI_DEVICE_PROTOCOL_UMP_MIDI_1_0_UP_TO_64_BITS: AMidiDevice_Protocol =
        AMidiDevice_Protocol(1);
}
// ...
impl AMidiDevice_Protocol {
    pub const AMIDI_DEVICE_PROTOCOL_UNKNOWN: AMidiDevice_Protocol = AMidiDevice_Protocol(-1);
}
#[repr(transparent)]
#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
pub struct AMidiDevice_Protocol(pub i32);

And with some bindgen tweaking we might do even better (and there's a special representation for bitflags types as well).

[DanAlbert]

The improved UX of a typed enum means we can type it in Rust as well, instead of currently having this untyped "mess"

I hadn't actually thought about the problem from this angle before. Filed #1741 to track it.

[DanAlbert]

Finally has both the review it needs and has passed presubmit. We still need to manually pull the update into the NDK before it will be available in even a canary build.

[MarijnS95]

https://android-review.googlesource.com/q/project:platform/ndk+branch:master @DanAlbert where can we find the change?

[DanAlbert]

Nowhere until I pull that update. It had to land in an internal branch to avoid merge conflicts.

[DanAlbert]

https://android-review.googlesource.com/c/platform/prebuilts/ndk/+/2182695 has the updated sysroot. It's in presubmit right now but will make it into https://ci.android.com/builds/branches/aosp-master-ndk/grid? shortly after that merges. I'm going to get that (and a few other fixes) cherry-picked into r25b and send that off to QA soonish (a build should be visible in ci.android.com, but QA schedules are almost always the longest part of the release).