Skip to content

Conversation

@max99x
Copy link
Contributor

@max99x max99x commented Aug 25, 2011

This is a rather varied pull request. There is 1 significant fix and a lot of little ones.

The main change is that I've updated varargs handling to use the same approach on both internal and external or library functions. The main reason for this is that previously when a shared library called a varargs function defined in its parent, it passed its arguments in JS style, while the actual function received them in C style. This broke many Python extension modules to some degree. This is a rather extensive change as it required me to update every usage of varargs in the library. However, the end result was actually less code. I've added a few new tests to check that all this didn't break anything. Note also that the change uncovered a couple of problems in the tests, which I took the liberty of fixing:

  • test_bigint and test_printf passed around large 64-bit values, which get truncated to 32 bits when using USE_TYPED_ARRAYS. Printing them worked just because they were passed directly to printf() rather than stored in memory. Now that varargs are stored on the stack, they get truncated in typed arrays. For this reason, I've added a check to skip these two tests in builds that use typed arrays.
  • The trunc case in test_cases had an incorrecly declared printf() because it was compiled without the right headers.

Then there are the generic fixes:

  • Fixed v8/d8 path edge case (Issue Cannot find settings file when compiling with v8 #64).
  • Prevented quoting of library object properties. This prevents Closure Compiler advanced optimization from wrecking the library.
  • Made sure all preamble functions are exported. These are usually necessary for accessing the code from user JS (e.g. converting to/from C-style strings) and leaving them unexported means we have to explicitly specify them when compiling with Closure.
  • Cosmetic changes to eliminator script. No change in functionality.
  • Fixed usage of parent global vars inside shared libraries.
  • Added an "ignore" parameter to makeGetValue() for use in the varargs-driven printf()/_formatString() implementation.
  • Made sure SAFE_HEAP allows storing of NaNs (e.g. returns from math functions). The problem with this was uncovered by test_math_hyperbolic after the varargs changes, since previously NaNs were passed directly rather than stored in memory.
  • Moved errno buffer out of $FS and back into __setErrNo, with a static postset.
  • Moved getc()/putc() buffers back to getc()/putc()and switched them to be allocated on the stack. I'm assuming you moved them out into a static allocation inside$FS` because normal allocations caused autodebugger problems. I'm also assuming that stack allocations are Ok since you turned some normal allocations to stack ones in the same commit. If either of this is not the case, let me know.

Tested successfully with llvm_gcc_0_0, llvm_gcc_1_1, clang_0_0 and clang_1_1.

src/jsifier.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please add an explanation regarding why they should not be declared.

src/library.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please combine all three of these lines into one line.

Style fixes in response to code review.
@kripken
Copy link
Member

kripken commented Aug 25, 2011

I am concerned about the getc/putc change. They will now allocate 1 character in each call to getc/putc. If you read 10,000 characters using getc, you will be growing the stack by 10,000 and this also adds some CPU overhead. (This happens since the stack is unwinded only in a compiled function - not in getc/putc which are handwritten - for speed reasons). Creating a fixed statically-allocated buffer is preferable I think. If you have a better place to do the allocation than in FS.init then that is fine of course.

(Alternatively we can unwind the stack in getc/putc, but that would be slow I think.)

@max99x
Copy link
Contributor Author

max99x commented Aug 25, 2011

Fixed all. Also, mind-reading.

@kripken
Copy link
Member

kripken commented Aug 25, 2011

Looks great. Can you just verify that say llvm_gcc_0_0 tests pass, to make sure the last changes don't break anything)?

@max99x
Copy link
Contributor Author

max99x commented Aug 25, 2011

Verified llvm_gcc_0_0 to pass after this last commit.

kripken added a commit that referenced this pull request Aug 25, 2011
Varargs for externals, shared lib globals and other fixes
@kripken kripken merged commit 5b3b4c5 into emscripten-core:master Aug 25, 2011
lewing pushed a commit to lewing/emscripten that referenced this pull request Mar 14, 2025
…429.1 (emscripten-core#71)

[dotnet/main] Update dependencies from dotnet/arcade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants