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

Fix: Replace attribute alias in CRC32 #1713

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

ALTracer
Copy link
Contributor

@ALTracer ALTracer commented Jan 4, 2024

Detailed description

  • Not a new feature.
  • The existing problem is MacOS/darwin failing to build from source due to aliases are not supported on darwin after Fix: qCRC performance uplift #1708
  • This PR solves the problem by replacing the mechanism used to keep distinct function names.

During writing PR1708 I applied the DEBUG_ERROR("%s: ...", __func__, ...) idiom, removing the generic_crc32 name message-string from stm32_crc32() renamed implementation in the process. To keep the proper function names and avoid complicating the single callsite in gdb_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 on arm-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

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

Closing issues

* Aliases are not supported on darwin
* Mark actual impls for `static` linkage so that they get inlined
@dragonmux dragonmux added this to the v2.0 release milestone Jan 4, 2024
@dragonmux dragonmux added Bug Confirmed bug Regression Bug caused by a regression labels Jan 4, 2024
Copy link
Member

@dragonmux dragonmux left a 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.

src/crc32.c Show resolved Hide resolved
@ALTracer
Copy link
Contributor Author

ALTracer commented Jan 4, 2024

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 bmd_crc32() function). I can move the new fully duplicate perf log code into it, making the implementations themselves cleaner/drier and changing their clobbered register allocation / stack spillage. And then any instrumenting stack profilers get a chance at doing something before that big stack allocation (i.e. check stack depth at current moment or into the future, trigger errors etc.)

@dragonmux
Copy link
Member

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.

Copy link
Member

@dragonmux dragonmux left a 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.

@dragonmux
Copy link
Member

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.

@dragonmux dragonmux merged commit 8402360 into blackmagic-debug:main Jan 4, 2024
6 checks passed
@ALTracer ALTracer deleted the fix/crc32-alias branch January 5, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bug Regression Bug caused by a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants