Skip to content

Alpine aarch64 build questions #1645

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

Downchuck
Copy link

Summary

Running cmake --build ./build on a stock Alpine Linux installation with clang 17 (and clang 18) resulted in two compile errors - I am not clear if there's a simple environment fix or if it just needs more actual code in the Hermes repo.

lseek has lseek64 capabilities, but neither lseek or lseek64 are in the namespace scope. (raw_ostream.cpp)
thread-safety-analysis is not picking up capability annotations for std:mutex and so the analysis fails on the lock.

They're very simple and the patch just shows an inline workaround (e.g. calling lseek instead of lseek64, and for the sake of compiling, just disabling tsa as that source is already verified by other build environments)

Test Plan

This code presents the two issues and work-arounds, and is request for clarification as to the correct environment or appropriate workarounds:

lseek64 and sleek are not in the global namespace, and it may be that lseek is desired though the capability is 64.
This may have some clue as to that evolution:
https://reviews.llvm.org/D139752?id=484363

The failure of the TSA attribute is seen here:

/tmp/scratch/hermes/API/hermes/cdp/DomainAgent.h:67:39: warning: 'guarded_by' attribute requires arguments whose type is annotated with 'capability' attribute; type here is 'std::mutex' [-Wthread-safety-attributes]
   67 |     std::function<void(Args...)> func TSA_GUARDED_BY(mutex);
      |                                       ^
/tmp/scratch/hermes/API/hermes/../hermes/ThreadSafetyAnalysis.h:26:61: note: expanded from macro 'TSA_GUARDED_BY'
   26 | #define TSA_GUARDED_BY(x) TSA_THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))
      |                                                             ^
/tmp/scratch/hermes/API/hermes/cdp/DomainAgent.h:47:25: error: reading variable 'func' requires holding mutex 'funcContainer_->mutex' [-Werror,-Wthread-safety-analysis]
   47 |     if (funcContainer_->func) {

While this couple line patch works fine to get things going - it would be nice to understand if there's an appropriate patch or change to the environment. I've not checked a separate aarch64 Linux distribution with LLVM. I've not checked for pre-built releases from Hermes for aarch64. React Native 0.76 does not supply aarch64.

With this small patch I believe I'm building just fine end-to-end for RN 0.76 on aarch64. This patch is not intended to be merged upstream, but rather illustrates the only two issues I hit in compiling.

HAVE_LSEEK64 is triggering true, in musl aarch64 llvm they are just using sleek, and the global namespace is also having an error
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 10, 2025
@tmikov
Copy link
Contributor

tmikov commented Mar 10, 2025

Hi, we are still looking into the lseek64() issue, but with regards to TSA, we think we should just turn it off by default. Would you please submit a PR changing HERMES_THREAD_SAFETY_ANALYSIS to OFF in the root CMakeLists.txt?

@dannysu
Copy link
Contributor

dannysu commented Mar 10, 2025

hi @Downchuck, for the thread safety and for the changes under cdp, could you please make a dedicated PR? We'll review it.

For thread safety, rather than changing it to not error, could you please change https://github.com/facebook/hermes/blob/main/CMakeLists.txt to turn HERMES_THREAD_SAFETY_ANALYSIS to OFF by default?

Thanks

@Downchuck
Copy link
Author

Downchuck commented Mar 15, 2025

@dannysu @tmikov I think for lseek this one is fine for adding to the file:

#ifdef HAVE_LSEEK64
#ifndef lseek64
off64_t lseek64(int fd, off64_t offset, int whence) {
  return lseek(fd, offset, whence);
}
#endif
#endif

Per the lseek documentation.
I'll put up the two separate PRs, one for external/llvh/lib/Support/raw_ostream.cpp and one for CMakeLists.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants