-
-
Notifications
You must be signed in to change notification settings - Fork 781
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
Fix: Replace attribute alias in CRC32 #1713
Conversation
* Aliases are not supported on darwin * Mark actual impls for `static` linkage so that they get inlined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One query before we merge this - once we've got an answer, whatever that may be, we'll be happy to merge this.
Now that I think harder about it, and we got a common function in this path anyways (which the compiler doesn't feel like inlining), I got a new idea. Let's embrace it (the |
Ooh, sure, that works! We were going to suggest experimenting with an approach like in target_probe.c, but we like this idea way better. |
6f1f2e1
to
986c01f
Compare
986c01f
to
12c0da7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, don't see anything that needs addressing with this revised approach. Merging once the builds finish.
Also, a note to self for the future: Once Meson branch lands, implement macOS CI so we can catch mistakes like this in the future. |
Detailed description
aliases are not supported on darwin
after Fix: qCRC performance uplift #1708During writing PR1708 I applied the
DEBUG_ERROR("%s: ...", __func__, ...)
idiom, removing thegeneric_crc32
name message-string fromstm32_crc32()
renamed implementation in the process. To keep the proper function names and avoid complicating the single callsite ingdb_main.c
, I introduced a GCC-specific extension__attribute__((alias("")))
to substitute jumps at link time. This does not fly on MacOS (Mach-O limitation vs. ELF), and I'm unable to test for that desktop platform. Note that this is fine onarm-none-eabi-
adapters and Linux desktop, also MSYS2 Windows desktop (all of which are checked by CI). No comment on *BSD.Also mark actual impls for
static
linkage so that they get inlined, it saves a jump.Your checklist for this pull request
make PROBE_HOST=native
)make PROBE_HOST=hosted
)Closing issues