Page MenuHomePhabricator

[WebAssembly] Handle exception specifications
ClosedPublic

Authored by aheejin on May 8 2020, 4:23 PM.

Details

Summary

Wasm currently does not fully handle exception specifications. Rather
than crashing, this treats throw() in the same way as noexcept, and
ignores and prints a warning for throw(type, ..), for a temporary
measure.

Diff Detail

Event Timeline

aheejin created this revision.May 8 2020, 4:23 PM
aheejin marked an inline comment as done.May 8 2020, 5:12 PM
aheejin added inline comments.
clang/lib/CodeGen/CGException.cpp
23

One thing I'm not sure about is if it is a good thing to include this in CodeGen. Usually this header is used in Sema/ directory. This depends on which .td file I add warn_wasm_exception_spec_ignored. DiagnosticSemaKinds.td seemed to have a section in exception spec so I added it there, but not entirely sure if that's the best location to add it.

aheejin marked an inline comment as done.May 8 2020, 5:16 PM
aheejin added inline comments.
clang/lib/CodeGen/CGException.cpp
23

Oh, and this warning message was requested by Adobe, but I think it's good to have in general.

aheejin updated this revision to Diff 263218.May 11 2020, 10:59 AM
  • Delete a debugging(?) line
aheejin marked an inline comment as done.May 11 2020, 11:00 AM
aheejin added inline comments.
clang/test/CodeGenCXX/wasm-eh.cpp
406

This was preexisting just moved

Actually, would it be possible to not ignore throw() but make it an alias for noexcept(true)? Apparently that is the standard behavior in C++17, so it might make more sense to just implement that now rather than just warning all the time and ignoring it. It would also cover most of the cases that exist, so more users wouldn't need to disable the warning.

clang/lib/CodeGen/CGException.cpp
23

According to the CMakeLists.txt in lib/CodeGen, clangCodeGen depends on clangBasic, so I think it should be ok to include stuff from clang/Basic here (even if it has Sema in the name).
And a warning seems ok, as long as it's possible to suppress it.

aheejin retitled this revision from [WebAssembly] Ignore exception specifications to [WebAssembly] Handle exception specifications.May 15 2020, 1:52 PM
aheejin edited the summary of this revision. (Show Details)

Now this handles throw() in the same way as noexcept, and prints a warning only in case of throw(type, ..), as you suggested.

aheejin updated this revision to Diff 264335.May 15 2020, 1:54 PM

Handle throw() in the same way as noexcept

dschuff accepted this revision.May 15 2020, 2:56 PM
dschuff added inline comments.
clang/test/CodeGenCXX/wasm-eh.cpp
406

Is it common in these tests to have RUN lines throughout the file instead of all together up at the top?

This revision is now accepted and ready to land.May 15 2020, 2:56 PM
aheejin marked an inline comment as done.May 15 2020, 9:13 PM
aheejin added inline comments.
clang/test/CodeGenCXX/wasm-eh.cpp
406

Not sure how common it is, but we have similar examples, such as https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/wasm-toolchain.c. I moved them mainly for readability; because now we can check what prefix WARNING's command line is like and what WARNING's check lines like, and the same for ASSEMBLY. I don't have a strong opinion for this, so if you think it's better to merge them at the top, please let me know, I'll do that as a follow-up.

This revision was automatically updated to reflect the committed changes.

Committed and reverted this. Resubmitted this in D80061.

The revert broke check-clang on several bots, e.g. here http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/38159

Can you take a look please? The tree's been red all of today due to this.

thakis added inline comments.May 16 2020, 5:30 PM
clang/test/CodeGenCXX/wasm-eh.cpp
387

Ah, the problem is that this line is missing a -o flag and so it wrote a wasm-eh.ll file to the clang/test/CodeGenCXX directory, which on the next build was interpreted as a test without a run line. I'll land a temporary rm here to remove that file. Please fix this for the reland.

This is very strange. I haven't committed a file called wasm-eh.ll. (There is a file called wasm-eh.cpp, which existed for more than a year now.) Also there's no wasm-eh.ll in LLVM repo now either. I wonder how this happened..? Also I can't reproduce the bug in my local machine.

aheejin marked an inline comment as done.May 16 2020, 5:39 PM
aheejin added inline comments.
clang/test/CodeGenCXX/wasm-eh.cpp
387

Oh I see... Thanks a lot for doing this.