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

gh-89640: properly detect float word ordering on Linux #125571

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Oct 16, 2024

@erlend-aasland erlend-aasland marked this pull request as draft October 16, 2024 08:05
@erlend-aasland
Copy link
Contributor Author

@damelang, FYI I get this error on WASI:

grep: conftest.wasm: No such file or directory
grep: conftest.wasm: No such file or directory

At first, I thought using $EXEEXT was incorrect, and swapped it with .$ac_objext in 2465535, but my guess was wrong. Since the grep block is run only in case of success, I don't really see any other failure than the macro trying to grep the incorrect file name. Thoughts? I'll try to set up a WASM environment and dig deeper.

@damelang
Copy link

damelang commented Oct 17, 2024

@damelang, FYI I get this error on WASI:

grep: conftest.wasm: No such file or directory
grep: conftest.wasm: No such file or directory

At first, I thought using $EXEEXT was incorrect, and swapped it with .$ac_objext in 2465535, but my guess was wrong. Since the grep block is run only in case of success, I don't really see any other failure than the macro trying to grep the incorrect file name. Thoughts? I'll try to set up a WASM environment and dig deeper.

Hi Erlend, I may be stating the obvious, but it looks like your use of autoconf with a WASM compiler (clang?) is misconfigured. $EXEEXT is set to ".wasm" somewhere in the configuration, but the compiler is generating a binary with something else as the extension (maybe ".wasi", or quite possible no extension at all, in which case $EXEEXT maybe should be set to the empty string?).

I've never built an autoconf-based project with a WASM compiler before, and maybe it's not common enough of a situation for others to have run into this issue. It's not a great time for me to dive into this, but if I did, I'd poke around to see where $EXEEXT is set to ".wasm". And I'd look at what extension the compiler is actually giving the WASI binaries. One of those hopefully makes more sense to use than the other, and you can simply update the configuration/code to only use that one.

I wish I knew internals well enough to tell you where to go poking around 🤷🏼‍♂️

@erlend-aasland
Copy link
Contributor Author

It's not a great time for me to dive into this, but if I did, I'd poke around to see where $EXEEXT is set to ".wasm".

I've got a semi-functional WASI dev environment up and running with GitHub Codespaces, and I notice a few things:

  • in configure.ac, we mess up EXEEXT around L1323-1340; this code ends up around L7300 in the generated configure script, several thousands of lines after the compiler and linker helpers are defined 🤦
  • using --with-suffix=no works ✅
  • mirroring EXEEXT to ac_exeext after our configure.ac EXEEXT hacks in L1323-1340 works ✅

Seems our EXEEXT hacks are the source of the problem.

@erlend-aasland
Copy link
Contributor Author

Seems our EXEEXT hacks are the source of the problem.

From the top of my head, I see two options:

  • in configure.ac after L1340, set ac_exeext=$EXEEXT (adding yet another hack)
  • don't use EXEEXT; replace it with something outside of Autoconf's namespace, for example EXE_SUFFIX

Let's go for the latter. I'll create an issue and a PR.

@erlend-aasland erlend-aasland enabled auto-merge (squash) October 26, 2024 15:18
@erlend-aasland erlend-aasland added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Oct 26, 2024
@erlend-aasland erlend-aasland merged commit 26d6277 into python:main Oct 26, 2024
40 checks passed
@erlend-aasland erlend-aasland deleted the autoconf/detect-float-word-ordering-on-linux branch October 26, 2024 15:46
@miss-islington-app

This comment was marked as outdated.

@miss-islington-app
Copy link

Sorry, @erlend-aasland, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 26d627779f79d8d5650fe7be348432eccc28f8f9 3.13

@miss-islington-app
Copy link

Sorry, @erlend-aasland, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 26d627779f79d8d5650fe7be348432eccc28f8f9 3.12

@erlend-aasland
Copy link
Contributor Author

If we are to backport this, we need to first backport #124657.

@hoodmane
Copy link
Contributor

Hmm this change broke float word order detection on Emscripten, because EEXT is conftest.js which does not contain the floats but the file with the float order is conftest.wasm. Presumably we can hard code float word order is little endian for Emscripten, though it would be nice if autoconf knew this...

@hoodmane
Copy link
Contributor

What about checking both the object file and the executable and setting the endian order if a match is found in either?

@erlend-aasland
Copy link
Contributor Author

@hoodmane, is there an Emscripten buildbot? How can I reproduce the failure?

@hoodmane
Copy link
Contributor

hoodmane commented Oct 30, 2024

No, I'm working on setting up a buildbot for Emscripten again after the steering council approved switching adding back tier 3 support for it last week.

Applying this patch to aclocal.m4 and running autoconf fixes the problem:

--- a/aclocal.m4
+++ b/aclocal.m4
@@ -112,10 +112,10 @@ int main (int argc, char *argv[])
 
 ]])], [
 
-if grep noonsees conftest$EXEEXT >/dev/null ; then
+if grep noonsees conftest$EXEEXT >/dev/null || grep noonsees conftest.$ac_objext >/dev/null; then
   ax_cv_c_float_words_bigendian=yes
 fi
-if grep seesnoon conftest$EXEEXT >/dev/null ; then
+if grep seesnoon conftest$EXEEXT >/dev/null || grep seesnoon conftest.$ac_objext >/dev/null; then
   if test "$ax_cv_c_float_words_bigendian" = unknown; then
     ax_cv_c_float_words_bigendian=no
   else

For now, to reproduce:

  1. Check out this branch https://github.com/hoodmane/cpython/tree/emscripten-autoconf-float-word-order
  2. Get emcc on the path with:
    git clone https://github.com/emscripten-core/emsdk --depth 1
    ./emsdk/emsdk install 3.1.68
    ./emsdk/emsdk activate 3.1.68
    source ./emsdk/emsdk_env.sh
  3. Use the script I added on my branch to build the "build python":
    python3.12 ./Tools/wasm/emscripten.py configure-build-python
    python3.12 ./Tools/wasm/emscripten.py make-build-python
  4. Use the script to configure the "host python". This should fail with the error I indicated.
    python3.12 ./Tools/wasm/emscripten.py configure-host

@hoodmane
Copy link
Contributor

Nevermind, the patch doesn't work because AC_LINK_IFELSE compiles and links in one command so there is no object file in the file system to grep. I'm not sure why I thought it worked.

@erlend-aasland

This comment was marked as resolved.

@erlend-aasland
Copy link
Contributor Author

@hoodmane, I can confirm that this breaks Emscripten. I'll reopen the issue. Perhaps a better solution is to directly manipulate BUILDEXEEXT instead of EXEEXT.

@hoodmane
Copy link
Contributor

What does BUILDEXEEXT mean? There's a comment about on case insensitive platforms the suffix can't be empty? Not clear to me how it helps here, but we could add

AS_CASE([$ac_sys_system],
   [Emscripten], [EXTTOINSPECT=.wasm],
   [EXTTOINSPECT=$EXEEXT]
)

and then look at conftest$EXTTOINSPECT. Maybe I'll open a PR doing this?

@erlend-aasland
Copy link
Contributor Author

What does BUILDEXEEXT mean?

It's the suffix the Python executable gets in the Makefile.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Nov 5, 2024
…ython#126387)

Properly detect float word ordering on Linux (pythongh-125571)

autoconf-archive patch by Dan Amelang.

(cherry picked from commit 26d6277)

Hardcode WASM float word ordering to little endian (pythongh-126387)

(cherry picked from commit 532fc08)
erlend-aasland added a commit that referenced this pull request Nov 5, 2024
…26429)

Properly detect float word ordering on Linux (gh-125571)

autoconf-archive patch by Dan Amelang.

(cherry picked from commit 26d6277)

Hardcode WASM float word ordering to little endian (gh-126387)

(cherry picked from commit 532fc08)
erlend-aasland added a commit that referenced this pull request Nov 5, 2024
…26430)

Properly detect float word ordering on Linux (gh-125571)

autoconf-archive patch by Dan Amelang.

(cherry picked from commit 26d6277)

Hardcode WASM float word ordering to little endian (gh-126387)

(cherry picked from commit 532fc08)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants