[BUG] `surface_control.h` and `font.h` contains C++ constructs #1920

MarijnS95 · 2023-08-16T12:38:07Z

Description

I maintain the Rust ndk and ndk-sys crates, and they rely on bindgen to parse C headers.

Unfortunately, while attempting to generate these for the font headers and later surface_control.h Rust's bindgen utility complains that the headers are not valid C (these things only work in C++):

~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:102:18: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:124:50: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:194:32: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:204:27: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:218:45: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:232:39: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:265:33: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:280:33: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font.h:295:32: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font_matcher.h:135:1: error: must use 'struct' tag to refer to type 'AFontMatcher'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font_matcher.h:144:27: error: must use 'struct' tag to refer to type 'AFontMatcher'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font_matcher.h:159:9: error: must use 'struct' tag to refer to type 'AFontMatcher'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font_matcher.h:175:9: error: must use 'struct' tag to refer to type 'AFontMatcher'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font_matcher.h:190:9: error: must use 'struct' tag to refer to type 'AFontMatcher'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font_matcher.h:211:1: error: must use 'struct' tag to refer to type 'AFont'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/font_matcher.h:212:15: error: must use 'struct' tag to refer to type 'AFontMatcher'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/surface_control.h:343:57: error: C does not support default arguments
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/surface_control.h:355:48: error: must use 'enum' tag to refer to type 'ADataSpace'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/surface_control.h:379:83: error: expected ')'
~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/surface_control.h:378:37: note: to match this '('

Missing `struct` and `enum` tags

These are easy enough (but ugly) to work around with typedefs:

typedef enum ADataSpace ADataSpace;
typedef struct AFont AFont;
typedef struct AFontMatcher AFontMatcher;
typedef struct ASystemFontIterator ASystemFontIterator;

Language features we cannot work around

Default function arguments

~/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/surface_control.h:343:57:

void ASurfaceTransaction_setBuffer(ASurfaceTransaction* transaction,
                                   ASurfaceControl* surface_control, AHardwareBuffer* buffer,
                                   int acquire_fence_fd = -1) __INTRODUCED_IN(29);

C++ references instead of pointers

/home/marijn/Android/Sdk/ndk/26.0.10404224/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/surface_control.h:379:83 (and at more places):

void ASurfaceTransaction_setGeometry(ASurfaceTransaction* transaction,
                                     ASurfaceControl* surface_control, const ARect& source,
                                     const ARect& destination, int32_t transform)
                                     __INTRODUCED_IN(29);

Affected versions

r25, r26

Canary version

No response

Host OS

Linux

Host OS version

Arch Linux

Affected ABIs

armeabi-v7a, arm64-v8a, x86, x86_64

Build system

Other (specify below)

Other build system

bindgen on headers only

minSdkVersion

Irrelevant for header codegen

Device API level

No response

The text was updated successfully, but these errors were encountered:

DanAlbert · 2023-08-16T18:29:57Z

Hmm. I'd like to fix this in r26 but that particular component has gotten tricky to update now that rv64 is in the development branch. It isn't a regression in r26, so this may slip to r27. I don't think that would be a problem for your use case because you could just update those projects from the canary anyway. Really you probably don't even need an NDK and could just pull the sysroot from ci.android.com.

enh-google · 2023-08-16T18:45:23Z

(and i guess this is another vote for "we really should get around to setting up the 'check that every header can be included in .c files' test" that comes up every year or so...)

MarijnS95 · 2023-10-19T07:14:18Z

Note that the same issue appears in the thermal headers as well:

/opt/android-sdk/ndk/26.1.10909125/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/thermal.h:114:53: error: must use 'enum' tag to refer to type 'AThermalStatus'
/opt/android-sdk/ndk/26.1.10909125/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/thermal.h:146:1: error: must use 'enum' tag to refer to type 'AThermalStatus'
panicked at 'Unable to generate bindings: ClangDiagnostic("/opt/android-sdk/ndk/26.1.10909125/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/thermal.h:114:53: error: must use 'enum' tag to refer to type 'AThermalStatus'\n/opt/android-sdk/ndk/26.1.10909125/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/android/thermal.h:146:1: error: must use 'enum' tag to refer to type 'AThermalStatus'\n")', bindgen-cli/main.rs:52:36

Not sure if you're aware and/or adding a CI process to check that all headers are C-compliant, so mentioning it here.

DanAlbert · 2023-10-19T18:42:59Z

Not sure if you're aware and/or adding a CI process to check that all headers are C-compliant, so mentioning it here.

We tried, but the build systems we have to work made it quite difficult.

MarijnS95 · 2024-01-04T09:50:30Z

Looks like the font headers have been fixed, but surface_control.h is still broken even on the latest CI builds. Can I contribute these changes upstream? I have decided to need and write a wrapper for SurfaceControl now, and continuously need to make trivial fixes to this header if I wish to regenerate/update our bindings.

On that note I'm very much inclined to follow @DanAlbert's advice and migrate the ndk-sys crate to NDK CI builds because of all the missing API from the published NDK as pointed out above.

@appujee what is your plan for the rust label? While this issue primarily impacts our ecosystem, the effect is global (headers aren't usable in plain C) and not limited to Rust.

MarijnS95 · 2024-01-06T20:58:45Z

@enh-google sure, I understand why this is the case, but wanted to report these issues as a whole and have a team of Googlers pick this up, rather than me painstakingly going through every API and requesting clarification everywhere, effectively picking up that job. Which I'm already doing to a significant extent, by using my free time to map NDK APIs into Rust, parsing the documentation, and finding various inconsistencies or general lack of clear and consistent documentation.

Sounds like I'll either have to leave it be, or file many separate issues on https://issuetracker.google.com/issues/new?component=192763&template=1032811 (assuming the NDK component is correct).

Notice that I'm trying to be helpful by pointing out lacking or inconsistent documentation, rather than figuring it out myself from the codebase and moving on with my day (with the added benefit of the semantics being locked in place). Asking a contributor to also take over part of someones job of triaging via git log, at that point I might as well end up on a payroll and submit documentation fixes upstream immediately (as already done once shown above).

The NDK APIs are supposed to be C-compatible. This one accidentally included a default argument, which is a C++ feature. That's being fixed, so this needs to pass the argument explicitly. Bug: android/ndk#1920 Test: treehugger Change-Id: I8e47a5cba8d485993c65e1b59e42ac5d7bd8d276

The NDK APIs are supposed to be C-compatible. This one accidentally included a default argument, which is a C++ feature. That's being fixed, so this needs to pass the argument explicitly. Bug: android/ndk#1920 Test: treehugger Change-Id: Ia79da4b4593dfd46302cb44298e906bb8e6c4d86

This is far from the only C incompatibility in this file, but it's the one that's most likely to not break builds, so I'm starting small. This is a source compat break, but not an ABI break. Bug: android/ndk#1920 Test: treehugger Change-Id: I5a32762fa61b1399fb354397479603858184ea05

Bug: android/ndk#1920 Test: treehugger Change-Id: Ib8296e7b9c86809d16fbde0787d016095a189387

This header currently cannot be processed by a C compiler as it contains C++ patterns, which prevent us from generating bindings in the [Rust `ndk-sys` crate]. This was originally reported at https://github.com/ android/ndk/issues/1920. The compatibility changes are as follows: - Remove `= -1` default-argument; - Suffix `ADataSpace` usage with `enum` tag. A word of warning: other bindings take this as an `int`, which is less descriptive. What is correct for these APIs, and can we unify it?; - Replace `&` references with `*` pointers, and marked them as `_Nonnull`. Confusingly lots of headers and functions here are lacking `_Nonnull`/`_Nullable` annotations, making our mapping "job" in Rust pretty terrible. Some improvements to the bindings: - Make HDR metadata struct pointers `const` as they are only used for reading; - Remove the duplicate `Available since API level 34.` on `ASurfaceTransaction_setExtendedRangeBrightness()` that was introduced in the wrong place in I62df5447bc547be52f6b00fedf1006c8d66a5faf, and later added again _after_ the parameter list in I8bebb31f0bd38e36139026a539030e5b61b75b4c. Note that the other notions _above_ the parameter list (also added by the first linked change) are kept; - Fix a typo reference to `SurfaceControlStats` as reported in [comment 14]. [Rust `ndk-sys` crate]: https://github.com/rust-mobile/ndk [comment 14]: https://issuetracker.google.com/issues/300602767#comment14 Change-Id: I9186df4abe7cb8246b2d10c1b41ab13664bad3ec Signed-off-by: Marijn Suijten <marijns95@gmail.com>

DanAlbert · 2024-06-04T20:04:54Z

Still a work in progress. It's going to take a while still. Moving to r28.

DanAlbert · 2024-08-01T20:48:13Z

Most of the issues have been cleaned up. The reference arguments are definitely still there though. I'd been saving those for last because it'd be a safety regression to do that without also adding _Nonnull, but now that the rest of the file is annotated we should be able to do that. I'll see how much more of this we can manage for r28, but this work has been extremely slow for infrastructural reasons, and I'm unfortunately spread very thin right now.

MarijnS95 · 2024-08-01T20:51:42Z

Understood; I've been spread equally thin (thinner than individual atoms, how is that possible) with no time to set up an AOSP tree to help you out. Either way thank you for pushing parts of my commits forward! I do hope that I can one day land the SurfaceControl APIs to our Rust ndk crate, with our without manually patching the headers :)

DanAlbert · 2024-08-01T21:04:08Z

The problems aren't thing a non-googler could help with anyway, unfortunately :( Making the change isn't the hard part, it's getting it past all the presubmit crap we have. And the post-submit stuff, for that matter.

DanAlbert · 2024-08-01T22:28:46Z

Actually we are way closer on this than I'd expected. At least for the set of headers you mentioned, it really is just the C++ references left in surface_control.h. There's also something in thermal.h (missing struct and enum tags) and performance_hint.h (missing #include <stdbool.h>). If the list is that short I'm somewhat optimistic that we can get this cleaned up for r28.

https://r.android.com/q/topic:%22surface_control-c-compat%22 fixes the rest of the originally filed problems. Getting that merged will probably take a few days since downstream branches always cause headaches.

DanAlbert · 2024-08-01T22:40:14Z

https://r.android.com/3205058 fixes performance_hint.h and https://r.android.com/3205910 for thermal.h. Those two should get through easily since they don't impact the ABI dumps.

We'll probably regress on this again. I'll take another stab at enforcing C compatibility at build time, but the last few times I tried it proved very difficult. I might be able to do something stupid but good enough instead.

DanAlbert · 2024-08-05T21:25:53Z

https://ci.android.com/builds/branches/aosp-master-ndk/grid?head=12185972&tail=12185972&legacy=1 and newer should be fixed. If it's not too much work for you to test the canary, want to do that and let me know if I missed anything bindgen needs?

MarijnS95 · 2024-08-05T21:35:30Z

Thanks @DanAlbert, that's awesome! Works flawlessly as demonstrated at rust-mobile/ndk@6175ea0.

Forgot to mention but your fix for thermal.h got me to bind AThermal over the weekend. performance_hint.h was already compiling for us (we pull in all the headers and one somewhat earlier must've already included stdbool.h) so that got bound as well. All that remains is an inhumane amount of source-code diving to understand and describe the semantics of the API (in particular around callbacks).

DanAlbert · 2024-08-05T21:43:14Z

Great, thanks for confirming.

I'll keep working on adding some validation to make sure we don't regress. Adding it in the right place to catch it early is the hard part, but if that doesn't pan out I suppose I can add something to the NDK tests themselves. If we catch it then, the mistake has already been made in the OS so it'll delay a release (potentially for a long time, seeing how long it took us to fix this) as opposed to preventing the broken change from landing in the first place, but that's better than shipping it broken.

All that remains is an inhumane amount of source-code diving to understand and describe the semantics of the API (in particular around callbacks).

File doc bugs anywhere the docs aren't clear and I'll see what I can do. The newer APIs should have somewhat okay docs (they go through fairly thorough review, but a lot of the wtfs aren't apparent until they start getting used in the wild), and I've been trying to improve some of the older docs. I expect there are several months of full time work fixing all the old stuff though, so some direction toward the worst offenders would help.

MarijnS95 · 2024-08-05T21:58:48Z

The doc bug I've filed thus far is probably too generic to gain any traction (likewise reporting general formatting and spelling mistakes is a dead end beyond contributing them myself, but that's a full-time job: https://issuetracker.google.com/issues/300602767).

Specifically, and perhaps this should be driven generically, it'd be nice to know when objects are internally synchronized inside Android, hence making them thread-safe. Specifically, in Rust this applies to the whole object, not just a few functions (though there are ways to represent such interactions too).
And also a more weak requirement, whether it's okay to migrate a pointer to a different thread (i.e. it's not tied to a thread-local of sorts).

Choreographer is a good example of this: it relies on a looper to be active on the current thread to query the instance: but can I pass the pointer on to other threads afterwards and (externally synchronized or not) register callbacks from there?

For callbacks the main issue I've been debugging a while back (and am a bit fuzzy on the details) is race conditions regarding when a callback is registered, replaced, removed, and the entire object being removed/destroyed, versus the callback being triggered. Unfortunately I've only vaguely recollected my thoughts in rust-mobile/ndk#464, and will need to run some experiments again.
The callback in Thermal is exceptionally nice because it's fully internally synchronized, _and its lifetime is tied to that of AThermalManager (if we release our object, the callbacks are removed as well).
ImageReader and perhaps also MediaCodec where the worst, where I could easily trigger race conditions.

The NDK APIs are supposed to be C-compatible. This one accidentally included a default argument, which is a C++ feature. That's being fixed, so this needs to pass the argument explicitly. Bug: android/ndk#1920 Test: treehugger Change-Id: Ia79da4b4593dfd46302cb44298e906bb8e6c4d86

MarijnS95 added the bug label Aug 16, 2023

MarijnS95 mentioned this issue Aug 16, 2023

ndk: Add bindings for AFont rust-mobile/ndk#397

Merged

This comment was marked as off-topic.

Sign in to view

appujee added the rust label Dec 26, 2023

This comment was marked as off-topic.

Sign in to view

MarijnS95 mentioned this issue Jan 18, 2024

[BUG] Missing symbol for ANativeWindow_get/setBuffersDataSpace() #1982

Closed

This comment was marked as off-topic.

Sign in to view

jrguzman-ms pushed a commit to msft-mirror-aosp/platform.frameworks.native that referenced this issue Apr 5, 2024

Make enum use C compatible.

dd05718

Bug: android/ndk#1920 Test: treehugger Change-Id: Ib8296e7b9c86809d16fbde0787d016095a189387

DanAlbert self-assigned this Aug 1, 2024

DanAlbert closed this as completed Aug 5, 2024

[BUG] surface_control.h and *font*.h contains C++ constructs #1920

[BUG] surface_control.h and *font*.h contains C++ constructs #1920

Comments

MarijnS95 commented Aug 16, 2023 • edited Loading

Description

Missing struct and enum tags

Language features we cannot work around

Default function arguments

C++ references instead of pointers

Affected versions

Canary version

Host OS

Host OS version

Affected ABIs

Build system

Other build system

minSdkVersion

Device API level

DanAlbert commented Aug 16, 2023

enh-google commented Aug 16, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

MarijnS95 commented Oct 19, 2023

DanAlbert commented Oct 19, 2023

This comment was marked as off-topic.

MarijnS95 commented Jan 4, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

MarijnS95 commented Jan 6, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

DanAlbert commented Jun 4, 2024

DanAlbert commented Aug 1, 2024

MarijnS95 commented Aug 1, 2024

DanAlbert commented Aug 1, 2024 • edited Loading

DanAlbert commented Aug 1, 2024

DanAlbert commented Aug 1, 2024

DanAlbert commented Aug 5, 2024

MarijnS95 commented Aug 5, 2024

DanAlbert commented Aug 5, 2024

MarijnS95 commented Aug 5, 2024 • edited Loading

[BUG] `surface_control.h` and `font.h` contains C++ constructs #1920

[BUG] `surface_control.h` and `font.h` contains C++ constructs #1920

MarijnS95 commented Aug 16, 2023 •

edited

Loading

Missing `struct` and `enum` tags

DanAlbert commented Aug 1, 2024 •

edited

Loading

MarijnS95 commented Aug 5, 2024 •

edited

Loading