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

android: add mallinfo2 #3940

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

android: add mallinfo2 #3940

wants to merge 1 commit into from

Conversation

SamB
Copy link

@SamB SamB commented Sep 24, 2024

It's actually just mallinfo in a funny hat, just like in C; see https://android-review.googlesource.com/c/platform/bionic/+/1910571 for the review of when they added this to Bionic's malloc.h header.

Since it's actually just mallinfo in dsguise, there is no minimum SDK level (just like for mallinfo).

Unlike in the header, I had to copy and paste the struct, because that's easier than building/borrowing a Rust macro to duplicate the fields. (Sometimes worse really is better.)

It's actually just mallinfo in a funny hat, just like in C; see
<https://android-review.googlesource.com/c/platform/bionic/+/1910571>
for the review of when they added this to Bionic's malloc.h header.

Since it's actually just mallinfo in dsguise, there is no
minimum SDK level (just like for mallinfo).

Unlike in the header, I had to copy and paste the struct, because
that's easier than building/borrowing a Rust macro to duplicate the
fields. (Sometimes worse really is better.)
@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@SamB
Copy link
Author

SamB commented Sep 24, 2024

Hmm, I was really hoping there was Android CI because there are a lot of funny little failures when I run "cargo test" on my phone (and that's after skipping the tests for sem_open/sem_close/sem_unlink that Termux #defines to names not found in libc.so).

Lucky for me, it looks like more checks materialized while I was writing this comment.

Well, the dynamic linker looks a bit grouchy, but the tests themselves appear to pass.

See e.g.
https://github.com/rust-lang/libc/actions/runs/11006374551/job/30560642282?pr=3940#step:4:972 for the linker complaints:

linker: /checkout/target/x86_64-linux-android/debug/deps/main-5f4c44db87887603: unsupported flags DT_FLAGS_1=0x8000001

No clue why the dynamic linker on my phone is happy with the same flags ...

@tgross35
Copy link
Contributor

This looks fine to me, @maurer mind double checking?

@maurer
Copy link

maurer commented Oct 14, 2024

Seems fine from an Android POV, that function is indeed available.

That said:

  1. Maybe we should use a type alias rather than a separate struct? Not a strong opinion here though.
  2. What are you looking for mallinfo2 for? malloc_info is more reliable and supported from API level 23, which accounts for 98.6% of existing devices. (It is true though, that rustc still supports back to API level 21.)

@tgross35
Copy link
Contributor

tgross35 commented Nov 6, 2024

Labeled waiting on author since the type alias makes sense here as Matthew mentioned (pub type mallinfo2 = mallinfo). Just comment @rustbot review once that is done.

What are you looking for mallinfo2 for? malloc_info is more reliable and supported from API level 23, which accounts for 98.6% of existing devices. (It is true though, that rustc still supports back to API level 21.)

It would be good to confirm that this is actually useful as well (easy enough if you are supporting an old API).

@tgross35
Copy link
Contributor

@SamB could you change this to make use of type aliases? Also confirm that this is for situations where malloc_info isn't available.

@bors
Copy link
Contributor

bors commented Nov 27, 2024

☔ The latest upstream changes (presumably #4132) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants