[clang-tools-extra] Ensure NoTrapAfterNoreturn is false for the wasm backend (PR #65876)

Heejin Ahn via cfe-commits cfe-commits at lists.llvm.org
Sun Sep 24 12:22:36 PDT 2023


aheejin wrote:

> > Can you point out what are the unsupported options here?
> 
> All of them, they are all options that translate to TargetOptions, and they do nothing for a wasm target triple: ` --trap-unreachable=false --xcoff-traceback-table=true --relax-elf-relocations=false --vec-extabi=true`

I don't think these options are the same case as this PR. These are just non-wasm options, so they mean nothing to wasm. Wasm simply doesn't do anything with respect to these options and wasm is not _overriding_ them. But with this PR, even if a user gives `--no-trap-after-noreturn` in the command line, they wouldn't know that their request was silently rejected/overridden.

To say it another way, the difference is, Wasm doesn't understand `--xcoff-traceback-table=true` and will do nothing with respect to it. But Wasm understands `--no-trap-after-noreturn` and will silently reject it after this PR.

> > I'm suggesting a warning only when an incompatible option is explicitly given in the command line.
> 
> I'm just trying to fix a bug and match how it currently works. If you want to change the behaviour to give more warnings, and especially only when an option is given on the command line, that's a more involved change that belongs in a separate issue.

I have been kind of confused by your characterization of this as "bugfix", given that this does not change anything functionality-wise. I agree this PR can be an improvement being a failsafe mechanism if someone chooses to override the default option or explicitly chooses to override it in the command line. But I think, at least in the latter case, the user should be informed that even though they requested `--no-trap-after-noreturn`, we will do `--no-trap-after-noreturn=0` regardless.

So I'm not sure if I agree that this PR is a bugfix. I consider this as an improvement adding a failsafe mechanism.

> > If you grep with report_fatal_error in all target's ***TargetMachine.cpp, there are many instances of reporting that some options are not supported. (I'm not necessarily suggesting erroring out though)
> 
> Most of these are hard errors, where there's a real contradiction between two requested features. Not so with TrapUnreachable and NoTrapAfterNoreturn, they are more like hints or suggestions, and can be ignored without breaking anything.

Didn't you provide a counterexample that `--no-trap-after-noreturn` produces invalid code? Then don't we have "a real contradiction"? What's your distinction criteria for "real contradiction" and "hints or suggestions"?


https://github.com/llvm/llvm-project/pull/65876


More information about the cfe-commits mailing list