Skip to content

Fixing asm2wasm crashing on \r when built with MinGW #986

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fixing asm2wasm crashing on \r when built with MinGW #986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05ea94e
Fixed asm2wasm crashing on files containing '\r' when built with MinGW
jerstlouis Apr 28, 2017
ad20584
Increasing stack when built with MinGW
jerstlouis Apr 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ ELSE()
ADD_COMPILE_FLAG("-Wno-unused-parameter")
ADD_COMPILE_FLAG("-fno-omit-frame-pointer")
IF(WIN32)
ADD_LINK_FLAG("-Wl,--stack,8388608")
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 a comment for this. i assume it's that unix stack sizes are normally 8MB but we need to add this for mingw?

Copy link
Author

@jerstlouis jerstlouis Apr 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Was just making it the same as the MSVC section. MinGW defaults to 2 MB I think.

ADD_COMPILE_FLAG("-D_GNU_SOURCE")
ELSE()
ADD_COMPILE_FLAG("-fPIC")
Expand Down
3 changes: 2 additions & 1 deletion src/support/file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ T wasm::read_file(const std::string &filename, Flags::BinaryOption binary, Flags
if (debug == Flags::Debug) std::cerr << "Loading '" << filename << "'..." << std::endl;
std::ifstream infile;
std::ios_base::openmode flags = std::ifstream::in;
if (binary == Flags::Binary) flags |= std::ifstream::binary;
// asm2wasm.exe built with MinGW would crash with files containing '\r' when opened without the binary flag
Copy link
Member

Choose a reason for hiding this comment

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

does anyone know why mingw is special here?

Copy link
Author

Choose a reason for hiding this comment

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

Is it possible that it's not MinGW specific but also an issue when built with Visual Studio?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but the reason I am guessing it's mingw-specific is that people have built and used this with vs. At least I think so, I didn't do it myself...

Copy link
Author

Choose a reason for hiding this comment

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

If the code already always handles skipping \r and works on Linux which does not have a non-binary mode, then there's no reason why the file should be opened without the binary flag, and I can't see how it would work better on Visual Studio.

There's other reports of asm2wasm crashing, see for example: #327 which crashes at exactly the same spot.

Copy link
Author

Choose a reason for hiding this comment

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

Why are \r even generated though? Are they only generated when done on Windows? It doesn't make sense to me to generate files meant to be used across platforms differently whether they're created on one platform or another. The 'text' mode and \r\n really needs to disappear anyway, it's such a horrible relic of the past.

Copy link
Member

@kripken kripken May 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think when we generate the asm.js text, on windows the text-handling python code in windows just emits \rs automatically. @juj knows more.

If there is no danger here, then opening as binary sounds ok, but I don't know enough about windows and text handling there.

Copy link
Author

Choose a reason for hiding this comment

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

Again opening as binary when outputting the asm.js will avoid those \r's. This will be an issue trying to get reproducible output across systems. Really, the 'text' mode has no reason to be.

/*if (binary == Flags::Binary) */flags |= std::ifstream::binary;
infile.open(filename, flags);
if (!infile.is_open()) {
std::cerr << "Failed opening '" << filename << "'" << std::endl;
Expand Down