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

Cygwin build fixes #1332

Merged
merged 3 commits into from Feb 11, 2020
Merged

Cygwin build fixes #1332

merged 3 commits into from Feb 11, 2020

Conversation

okuoku
Copy link
Contributor

@okuoku okuoku commented Feb 11, 2020

Build fixes to run wabt tools on Cygwin64. I haven't tested it much, but at least it runs DOOM okuoku/freestandoom#2 with the interpreter + WASM C-API :)

  • CMakeLists.txt -- Cygwin's libc(newlib) doesn't export POSIX API fileno under -std=c++11; use -std=gnu++11 instead.
  • binary-reader-objdump.cc: Use proper format string. Using %lu here would take some garbage, as argument is uint32_t. I guess it can be PRIu32 but followed other occurrences there.
  • (not Cygwin specific) decompiler.cc: Use unsigned 1U

On cygwin, `__STRICT_ANSI__` does not show POSIX definitions. Use
gnu++11 language instead.
Silence -Wsign-compare warning, by using unsigned literal one.
Use `%u` instead of `%lu` as we use `uint32_t` here.
Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

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

nice, thanks!

@binji binji merged commit 4ba97c1 into WebAssembly:master Feb 11, 2020
@okuoku okuoku deleted the cygwin-fix branch February 12, 2020 00:43
@sbc100
Copy link
Member

sbc100 commented Feb 12, 2020

Cool! Can you point to the code where use used the C-API? I'm keen to see it an action.

else()
# On Cygwin, POSIX extensions are not visible with std=c++11
# because it defines __STRICT_ANSI__.
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=gnu++11")
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to define _POSIX_SOURCE or something like that to signal that we want POSIX rather than asking for GNU extensions? I also don't think its a great idea to use different -std on different platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, there's room for improvement. Currently, the only POSIX consumers are

return isatty(fileno(file));
AND gtest. For the color.cc we'd just have _POSIX_C_SOURCE on it but I'm not sure how do we want to deal with gtest...

So I choose to limit gnu++11 to Cygwin and leave it as-is on the other (major) implementations such as glibc(Linux) or BSD libc.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to define _POSIX_C_SOURCE for the whole project (assuming that works) than turn on GNU extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll try. I found gtest upstream also have a proposed patch https://github.com/google/googletest/pull/2653/files so we should be able to do the same here.

The only culprit seems strcasecmp which is also POSIX. I'll chase why Cygwin does not export it under _POSIX_C_SOURCE=200809L.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed: #1333

CMakeLists.txt Show resolved Hide resolved
@okuoku
Copy link
Contributor Author

okuoku commented Feb 12, 2020

Can you point to the code where use used the C-API?

Not more than copy/pasted from hello.c: https://github.com/okuoku/freestandoom/blob/9150923c78a169d1563100fbc85a1302c0bab5df/host-common/callwasm.c

I found current C API implementation lacks wasm_module_imports API so I had to rely import order to expose native APIs to compiled WASM module. It seems the upstream does not have any example on the API anyway.

Currently it does not have any proper build system/instructions(sorry!) video instead: https://www.youtube.com/watch?v=EBF7E6_qkn8

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.

None yet

3 participants