Skip to content
This repository was archived by the owner on Jan 9, 2026. It is now read-only.

Rename data segment symbols, to avoid system library overrides#1184

Merged
jwiegley merged 3 commits intomasterfrom
johnw/alpha
Apr 7, 2023
Merged

Rename data segment symbols, to avoid system library overrides#1184
jwiegley merged 3 commits intomasterfrom
johnw/alpha

Conversation

@jwiegley
Copy link
Contributor

@jwiegley jwiegley commented Apr 6, 2023

On macOS, GHC 9.6 gives incorrect answers for transcendental functions, due to symbol overriding that happens from the math library. This PR fixes that problem with a simple alpha substitution.

Here is the total quantity of symbols in use by the Musl code; all of the rest that were not renamed are error conditions:

0000000000000000 T ___math_divzero
0000000000000000 T ___math_invalid
0000000000000000 T ___math_oflow
                 U ___math_xflow
0000000000000000 T ___math_uflow
                 U ___math_xflow
0000000000000000 T ___math_xflow
                 U ___kadena_exp_data
                 U ___math_oflow
                 U ___math_uflow
0000000000000000 T _musl_exp
0000000000000000 S ___kadena_exp_data
                 U ___kadena_log_data
                 U ___math_divzero
                 U ___math_invalid
0000000000000000 T _musl_log
0000000000000000 S ___kadena_log_data
                 U ___kadena_exp_data
                 U ___kadena_pow_log_data
                 U ___math_invalid
                 U ___math_oflow
                 U ___math_uflow
0000000000000000 T _musl_pow
0000000000000000 S ___kadena_pow_log_data
                 U ___kadena_rsqrt_tab
                 U ___math_invalid
0000000000000000 T _musl_sqrt
0000000000000000 S ___kadena_rsqrt_tab

@jwiegley jwiegley requested review from jmcardon and larskuhtz April 6, 2023 03:27
@jwiegley jwiegley self-assigned this Apr 6, 2023
@larskuhtz
Copy link
Contributor

Can we be sure that those error conditions can't conflict with existing symbols from -lm and possibly have an effect on-chain results?

@jwiegley
Copy link
Contributor Author

jwiegley commented Apr 6, 2023

@larskuhtz Ok, I've renamed them all:

0000000000000000 T ___kadena_math_divzero
0000000000000000 T ___kadena_math_invalid
0000000000000000 T ___kadena_math_oflow
                 U ___kadena_math_xflow
0000000000000000 T ___kadena_math_uflow
                 U ___kadena_math_xflow
0000000000000000 T ___kadena_math_xflow
                 U ___kadena_exp_data
                 U ___kadena_math_oflow
                 U ___kadena_math_uflow
0000000000000000 T _musl_exp
0000000000000000 S ___kadena_exp_data
                 U ___kadena_log_data
                 U ___kadena_math_divzero
                 U ___kadena_math_invalid
0000000000000000 T _musl_log
0000000000000000 S ___kadena_log_data
                 U ___kadena_exp_data
                 U ___kadena_math_invalid
                 U ___kadena_math_oflow
                 U ___kadena_math_uflow
                 U ___kadena_pow_log_data
0000000000000000 T _musl_pow
0000000000000000 S ___kadena_pow_log_data
                 U ___kadena_math_invalid
                 U ___kadena_rsqrt_tab
0000000000000000 T _musl_sqrt
0000000000000000 S ___kadena_rsqrt_tab

Also, the libm.h being included by the Musl code is from the same directory, it's not the system file.

@larskuhtz
Copy link
Contributor

larskuhtz commented Apr 6, 2023

I am somewhat concerned about conflicts when the system libm.h and the musl libm.h are both included, because AFAIK ghc tents to build all objects in a single invocation of gcc with lots of stuff included. Moving this code into a sub-library may possibly help to prevent that, because it would reduce isolate the build of this code from other parts of the package.

Some macros definitions in libm.h are wrapped into #ifndef ... so that previous definitions may hide or change definitions in headers that included later.

I think we should at least make sure that the environment can't overwrite definitions in our libm.h.

I am not 100% sure how justified this concern is. It seems that the content of libm.h is mostly the same across different implementations and that systems rarely change any of the macros that could affect behavior (like rounding mode). So, I am just siding in the cautious side here, possibly unnecessarily.

@jwiegley
Copy link
Contributor Author

jwiegley commented Apr 6, 2023

Also, this PR is an alternative to #1177, at least for fixing the GHC 9.6 build failure. There are other aspects of that PR that we'd like to include, but as a separate step to fixing the underlying issue.

@jwiegley
Copy link
Contributor Author

jwiegley commented Apr 6, 2023

@larskuhtz I don't quite understand your reticence with the locally provided libm.h. There are very few system headers being included (stdint, float and math), so all of these definitions should be entirely local to the Musl code, now that we've changed everything to have private names. I don't see how any outside libraries could now intercepts our definitions, unless they change definitions from the C stdlib headers.

@larskuhtz
Copy link
Contributor

The definitions in libm.h depend what value the environment provides for the following macros: LDBL_MANT_DIG, LDBL_MAX_EXP, TOINT_INTRINSICS, fp_barrierf, fp_barrier, fp_barrierl, fp_force_evalf, fp_force_eval, fp_force_evall. It's not completely obvious to me if these symbols cannot be defined in an upstream systems header like float.h or math.h or by the compiler (or component in the compilation environment like ghc or some other tool via -D) in a way that can affect the behavior of our code.

@jwiegley From inspecting the code it seems that unexpected definitions of above headers would likely either be redundant for our code or just cause compilation failures. But I am not an expert regarding these details of C implementations. If you think, that this isn't an issue and doesn't affect us, I am happy to approve this PR.

(I think, it would be good to isolate the code into an sub library and also to include the unit tests from #1177 for better maintainability and easier debugging of issues.)

@jwiegley
Copy link
Contributor Author

jwiegley commented Apr 7, 2023

@larskuhtz I'm not very worried about libm.h, since the number of new platforms we'll be encountering before this problem is resolved in other ways is so minimal.

I'd say let's get this in, fix your GHC 9.6 issue, and then once that is done I'd like to look at your restructuring code in more depth. After all, it may be that now is the time to embrace full Haskell implementations of these functions, or move to MPFR, rather than improving Musl. I still don't know why Musl fails Pact tests on Ubuntu/ARM64, but I'm going to try with this patch to see if what you ran into was the cause over there as well.

Copy link
Contributor

@emilypi emilypi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jwiegley jwiegley merged commit af84ccc into master Apr 7, 2023
@jwiegley jwiegley deleted the johnw/alpha branch April 7, 2023 18:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants