AX_C_FLOAT_WORDS_BIGENDIAN: fix bug with Emscripten target #316
Conversation
m4/ax_c_float_words_bigendian.m4
Outdated
| ax_cv_c_float_words_bigendian=yes | ||
| fi | ||
| if grep seesnoon conftest$EXEEXT >/dev/null ; then | ||
| if grep seesnoon conftest$TOINSPECTEXT >/dev/null ; then |
There was a problem hiding this comment.
Where does .wasm come from? Other Autoconf checks normally use conftest$ac_exeext or conftest$ac_ext. I believe that using one of these would be a better fix rather than special casing Emscripten in one in a multitude of Autoconf macros.
There was a problem hiding this comment.
Well what about falling back to the original check on the object file in that case?
There was a problem hiding this comment.
Where does
.wasmcome from?
Good question. My guess is that somewhere, someone decided that for Emscripten, the js file produced is the "executable" and the wasm file produced is the "object". Thus EXEEXT is ".js" and ac_objext is "wasm". If true, that design decision is the source of the problem here.
I, personally, would have considered the wasm file to be the executable, and the js file to be something different and perhaps unique to Emscripten, as a wrapper file or what not. Maybe there is some precedent already for similar targets that include a wrapper-ish file in the output, that Emscripten could have used. Anyway, if things had gone this way, EXEEXT would have been ".wasm" instead of ".js", the macro would have worked just fine as it is, this issue would not have come up, and all of us would have been spared some time and effort.
Alas, as things stand, we have this weird situation where, for Emscriptem only, AC_LINK_IFELSE apparently produces an object file along with an "executable", instead of just an executable. (Which might even be in counter to the documentation, perhaps.)
If I'm following things right (it is late for me), for Emscripten targets, this macro "needs" to look at conftest.$ac_object, which is conftest.wasm. But for everything else, it needs to look at conftest$EXEEXT.
I suggest a very simple fix: change the grep test to:
grep seesnoon conftest* >/dev/null
'cuz really...does anyone think that we'll get a false positive from some stray file that begins with conftest? IOW, that "seesnoon" or "noonsees" is found in some file not intended for this test, but does have a name that starts with conftest?
But in case that's too sloppy, we could do something a little more careful, like:
TESTFILES="conftest$EXEEXT $( [ -e conftest.$ac_objext ] && echo conftest.$ac_objext)"
and then:
grep seesnoon $TESTFILES ...
Make sense?
There was a problem hiding this comment.
I suggest a very simple fix: change the grep test to:
grep seesnoon conftest* >/dev/null'cuz really...does anyone think that we'll get a false positive from some stray file that begins with conftest? IOW, that "seesnoon" or "noonsees" is found in some file not intended for this test, but does have a name that starts with conftest?
I agree with you that this is probably good enough.
But in case that's too sloppy, we could do something a little more careful, like:
TESTFILES="conftest$EXEEXT $( [ -e conftest.$ac_objext ] && echo conftest.$ac_objext)"and then:
grep seesnoon $TESTFILES ...Make sense?
Yes; and that extra carefulness does not add significant complexity, so it seems to be the best solution.
There was a problem hiding this comment.
TESTFILES="conftest$EXEEXT $( [ -e conftest.$ac_objext ] && echo conftest.$ac_objext)"
This would not work because conftest.$ac_objext is conftest.o which does not exist if you run AC_LINK_IFELSE, that only makes conftest.js and conftest.wasm. To make conftest.o you need to also run AC_COMPILE_IFELSE and then you end up with something pretty similar to what I've already written. There isn't an environment variable set to .wasm.
But grep seesnoon conftest* >/dev/null would work.
There was a problem hiding this comment.
The object files are written
a.oso$ac_objextis.ofor Emscripten. We run the program likenode conftest.jsbut the fileconftest.wasmis necessary in order for the program to work. Thus,conftest.wasmis part of resulting program, not a build artifact. So if$EXEEXTis.jsthenAC_RUN_IFELSEworks correctly. OTOH if we changed$EXEEXTto be.wasm, it would make this particular macro work but all the ones that actually want to run the program would fail.Perhaps there is a standard way to deal with this situation, but I don't know of other targets where the executable consists of two files. It is possible to combine them into one file if we're willing to increase the size and startup time but it generally isn't worth it.
@sbc100 could perhaps provide additional context/advice.
I don't think I've groked all the context here but I don't think you wan to set $EXEEXT to .wasm since .wasm files on there own own are not runnable. In particular, as you have noted it would break the running on the executables during the build.
BTW, you can also leave EXEEXT as empty (like it is on UNIX) and emcc will produce a shell the .js file without and extension than then loads the wasm file. You might also want consider -sSINGLE_FILE which would then embed the wasm binaryn inside the js files.
There was a problem hiding this comment.
Though even that would probably fail the grep for the float since it gets base 64 encoded.
There was a problem hiding this comment.
The problem here is that the macro is trying to detect the endianness of floats by linking an executable with a float in it that consists of the ascii bytes noonsees or seesnoon depending on the platform endianness. In Emscripten, the .wasm file and the .o file have it but the .js executable does not.
There was a problem hiding this comment.
Given all that, it seems like grep seesnoon conftest* >/dev/null is the way to go, no?
Or to be less "sloppy", modify the TESTFILES approach to pick up the .wasm file if present:
CONFTESTS="conftest$EXEEXT $( [ -e conftest.wasm ] && echo conftest.wasm)"
But, I kinda prefer the generality of grep on conftest*. It's not wasm specific, so it could help with some other target someday also. And the "sloppiness" of it just doesn't seem to have any real risk.
The fact that it is a such a small change to the existing code is also appealing.
Just to make sure we're on the same page: the above statement is only true when referring to Emscripten targets. Even then, it's possible that it won't apply to later versions of Emscripten. Now, as a general statement: no, the float will not definitely live in an object file (in binary format). Per my documentation for the macro: "The problem was that under these conditions, the compiler did not allocate for and write the special float value in the data segment of the test object file." With the current version of the macro, there often isn't even an object file to inspect, only an executable. |
Agreed, I mean specifically for Emscripten and it could break in the future. But with no change the macro does not work with any combination of flags on the current version of Emscripten and it will continue to not work in the future. @damelang what about the first commit aa8ea4c? Would something like that be acceptable? Since on Emscripten targets the float will be in And what about the third option of hard coding Emscripten as little endian? Are any of these three solutions acceptable? |
I hope you find my suggested change above to be both more general and less complicated than either of those three solutions. |
1402d85 to
10c5b38
Compare
|
Okay @damelang I updated the PR with your proposed solution since we seem to agree on it. I'll make sure double check that it actually works before merging. |
Commit 23be7cc "Fix AX_C_FLOAT_WORDS_BIGENDIAN fails whenever interprocedural optimization is enabled." introduced a regression on the Emscripten target because it switched from grepping the object file where the float definitely will live, to grepping conftest$EXEEXT which in the case of Emscripten will be conftest.js but the float is in conftest.wasm. This problem is unique to Emscripten, it is not present on any other webassembly platform. To fix, grep `conftest*` for the float.
10c5b38 to
5a70049
Compare
|
Okay, I tested this and it works. |
|
I'm not a maintainer here, so now that we've got something we're all happy with, it's up to @peti to approve. @hoodmane , will you be moving the python build away from hard coding for wasm and using this approach instead? It would be helpful to have the python community "kick the tires" of the grep conftest* approach. |
|
Yeah I plan to do that. Thanks @damelang and @erlend-aasland! |
The original patch: autoconf-archive/autoconf-archive#279 broke endianness detection on Emscripten. This was fixed in this followup: autoconf-archive/autoconf-archive#316 We also have worked around the problem by adjusting `configure.ac` in the Python repo to handle this case: python/cpython#126387 But it's useful for us to use the upstream autoconf-archive detection so that we can be completely sure that the fix works in practice.
Fix endianness detection for Emscripten. Upstream patch: autoconf-archive/autoconf-archive#316
…nf-archive Pulls in the updated docker container from this PR: python/cpython-devcontainers#34 which contains the autoconf-archive PR: autoconf-archive/autoconf-archive#316 I removed the workaround from configure.ac, regenerated everything, and manually tested that the fix works.
…nf-archive Pulls in the updated docker container from this PR: python/cpython-devcontainers#34 which contains the autoconf-archive PR: autoconf-archive/autoconf-archive#316 I removed the workaround from configure.ac, regenerated everything, and manually tested that the fix works.
Commit 23be7cc "Fix AX_C_FLOAT_WORDS_BIGENDIAN fails whenever interprocedural optimization is enabled." introduced a regression on the Emscripten target because it switched from grepping the object file where the float definitely will live, to grepping aclocal$EXEEXT which in the case of Emscripten will be aclocal.js but the float is in aclocal.wasm. This problem is unique to Emscripten, it is not present on any other webassembly platform.
To fix, grep
conftest*for the float.