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

Remove umfMemoryTrackerGetAllocInfo() from def and map files #1221

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

Conversation

ldorau
Copy link
Contributor

@ldorau ldorau commented Mar 25, 2025

Description

Remove umfMemoryTrackerGetAllocInfo() from def and map files.

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly

@ldorau ldorau requested a review from a team as a code owner March 25, 2025 14:37
@lukaszstolarczuk
Copy link
Contributor

is this just a cleanup as it was never in the public API or did it change between versions?

@vinser52
Copy link
Contributor

is this just a cleanup as it was never in the public API or did it change between versions?

It is an ABI-breaking change.
The disjoint pool uses this function. But before @bratpiorka implemented the disjoint pool in C, the disjoint pool was a static library (not part of libumf.so) that is why libumf.so exported this function. I expect the compatibility test to fail for this PR.

@bratpiorka
Copy link
Contributor

We agreed to work on API cleanup for version 0.12x, and if we succeed, this could become our UMF 1.0. So this change is correct, but we should also update our compatibility workflows. We need to discuss what we want to achieve and probably update the tags in the compatibility workflows to 0.11-dev3 (this is the first tag with a disjoint pool in C).

@ldorau could you set this PR to draft?

@ldorau ldorau marked this pull request as draft March 26, 2025 08:09
@ldorau
Copy link
Contributor Author

ldorau commented Mar 26, 2025

Converted to draft

@lplewa
Copy link
Contributor

lplewa commented Mar 26, 2025

  1. Changing disjointpool from separate library to be part of libumf, was in fact API-braking change. This patch is just followup related to previous, change.
  2. This is our miss take, we should NEVER export internal functions from the library, so good that we are fixing it now.
  3. We decided that after 0.11 release we will do all braking changes, as preparation to 1.0 - so it's perfect time to fix problems like this.

@vinser52
Copy link
Contributor

vinser52 commented Mar 26, 2025

As part of API/ABI stabilization, I suggest introducing ABI testing as part of compatibility testing to explicitly detect when when our ABI is changing. I like what LLVM compiler does for that (See this for more information). And this is the LLVM rules to maintain stable API/ABI. I think we should follow the similar rules.

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

Successfully merging this pull request may close these issues.

5 participants