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

Reduce FPs when handling static inline functions #85

Closed
woodruffw opened this issue Apr 2, 2024 · 7 comments
Closed

Reduce FPs when handling static inline functions #85

woodruffw opened this issue Apr 2, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@woodruffw
Copy link
Member

See #55, #82, and #83.

abi3audit currently considers all symbols when auditing. This is generally correct, but results in false positives when a static inline function (such as Py_XDECREF) doesn't get inlined, but instead remains as a local/private symbol.

The cause below this is nuanced: static inline functions like Py_XDECREF are part of the limited API but not the stable ABI; they're expected to be inlined into code that is part of the stable ABI. In other words, static inline functions are referentially opaque: their expansion is compatible with the stable ABI, but function identifiers themselves are not.

In practice this is a non-issue, and abi3audit should not flag local-only symbols for static inline functions.

To do this, the audit phase probably needs two things:

  1. A list of static inline functions to ignore
  2. Each Symbol's visibility, to know whether to ignore it

For (1), we can just start with Py_XDECREF. For (2), I think we'll need to extend the abi3info Symbol model to include a visibility: Visibility | None attribute, which will need to be populated as appropriate from each supported symbol table/object file.

CC @nicholasjng, who helped triage this and has graciously offered to help out 🙂

@woodruffw woodruffw added the bug Something isn't working label Apr 2, 2024
@nicholasjng
Copy link
Contributor

Judging from that thread, the Python folks could use a collection tool for those inline methods and macros. Sounds like a fun project for getting into the cPython codebase.

fyi, you can also assign me directly here, I'm planning to work on this soon (until end of week).

@woodruffw
Copy link
Member Author

#87 fixes this in practice, but I'll leave this open for the longer-term potential fix of checking symbol visibility.

@nicholasjng
Copy link
Contributor

So regarding the symbol visibility, how do we fetch/infer the visibility from the stable_abi.toml file? That I would like to understand before starting the work on part 2).

@woodruffw
Copy link
Member Author

So regarding the symbol visibility, how do we fetch/infer the visibility from the stable_abi.toml file? That I would like to understand before starting the work on part 2).

I was thinking about it from the opposite direction 🙂 -- rather than trying to infer each function's intended visibility, we can look at actual symbol visibilities in the symbol table and apply the following rules:

  1. Symbol is abi3: pass
  2. Symbol is NOT abi3 but has local/private visibility: pass
  3. Symbol is NOT abi3 and has public visibility: fail

That way we'd end up allowing Py_XDECREF when it appears as a private symbol, but would still reject it if it (somehow) ends up being a public symbol. Thoughts?

@nicholasjng
Copy link
Contributor

Just for my understanding, public visibility means "linked in from an external lib", and "local/private" means from the project itself? How do I even get Py_XDECREF as a local/private visibility symbol?

@woodruffw
Copy link
Member Author

Just for my understanding, public visibility means "linked in from an external lib", and "local/private" means from the project itself? How do I even get Py_XDECREF as a local/private visibility symbol?

Yep, pretty much. The way you end up with a local/private Py_XDECREF is via a static member defined in a header, like CPython does.

As a contrived example:

// myheader.h
static inline int foo() { return 42; }
// mytool.c
#include <myheader.h>

expands roughly to:

// mytool.c
// myheader.h
static inline int foo() { return 42; }

In effect, this means that every translation unit (i.e. .c file, approximately) has its own local copy of the foo symbol, each of which is local (i.e. private). Normally the C compiler then inlines each of these foos (since they're inline) so the local-only foo goes away, but this is not guaranteed (especially with GCC's -Os, apparently).

TL;DR: this all happens a "public" header (like those in CPython) can define static members, which then become "private" in each C file they get expanded in. One of C's many joys 😅

@woodruffw
Copy link
Member Author

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants