aheejin wrote: > There's no difference between "doing nothing with respect to it" and > "silently rejecting", it's the same thing. The wasm backend understands > neither --xcoff-traceback-table, --trap-unreachable, _or_ > --no-trap-after-noreturn (the fact that --no-trap-after-noreturn affects the > wasm output despite being conceptually unsupported _is_ the bug). None of > those are supported by the wasm backend, and they are all equally silently > ignored.
I don't think they are the same thing. As I said above, `--xcoff-traceback-table` doesn't apply to Wasm because Wasm does not use xcoff. But `--trap-unreachable` or `--no-trap-after-noreturn` are applicable to wasm and we override them. I don't understand the analogy. > > I have been kind of confused by your characterization of this as "bugfix", > > given that this does not change anything functionality-wise. > > ...So I'm not sure if I agree that this PR is a bugfix. I consider this as > > an improvement adding a failsafe mechanism. > > It _does_ change things functionality wise. [Look at the > tests](https://github.com/llvm/llvm-project/pull/65876/commits/7e834694b09e6a7787674879442b5f0d0c92eb22#diff-1b973bc4d02665dc4a2d31916df466a5bf53c64ef70aa701ef579fc67dfbd2e4L96), > the output has changed with this pull request from invalid to valid. If > that's not a bug fix, I don't know what is. But `--no-trap-after-noreturn` didn't exist before, so there was no way to specify that from the command line. You _created_ it, originally in this PR, and then the split-off PR in #67051. If this is a bugfix, it sounds like you are fixing a bug of your own making. The reason I said this might count as an improvement was, in a hypothetical scenario when the default value for `NoTrapAfterNoreturn` changes, or someone else (if you didn't create that option) wants to add the same command line option, this overriding can guard us from those changes. But I think these scenarios are basically hypothetical and unlikely. So while I'm not opposed to these changes, I am confused by the motivation of this PR (together with #67051) and then more about the claim this is a bugfix. > > What's your distinction criteria for "real contradiction" and "hints or > > suggestions"? > > A real contradiction would be asking for two incompatible target options, > like [-exception-model=wasm > -enable-emscripten-cxx-exceptions](https://godbolt.org/z/n77WeqaT6). > TrapUnreachable is a much more wishy-washy request, it's frequently ignored > across the codebase, and [using it doesn't even guarantee that unreachable > will produce a trap](https://godbolt.org/z/evdrEvh1h). This is a case where the `unreachable` was optimized away or transformed to something else in `opt`, even before reaching the backend, so I don't think this counts as "we don't guarantee that`unreachable` will produce a trap". We (the Wasm backend) doesn't even see the `unreachable`. (Of course, it is entirely possible that we optimize out some code blocks containing `unreachable` and don't end up producing a `trap` corresponding to that within our Wasm backend. Anyway what I'm saying is optimizing away `unreachable` doesn't conflict the command-line option.) Also I'm having a hard time understanding your criteria between wishy-washy request and real contradiction still. To my understanding in both cases they are not compatible. > To put it plainly: the --trap-unreachable and --no-trap-after-noreturn > options _are not supported_ on the wasm backend, and they are silently > ignored just like every other unsupported option. This is consistent with the > behaviour both [in the wasm > backend](https://github.com/llvm/llvm-project/blob/21f8bc25adba762ab06e26a7dd50da78fcd17528/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp#L129), > and across the codebase: > [1](https://github.com/llvm/llvm-project/blob/aa8f5e6156821aec1fed595f2e13d69655ec3311/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp#L340), > > [2](https://github.com/llvm/llvm-project/blob/aa8f5e6156821aec1fed595f2e13d69655ec3311/llvm/lib/Target/ARM/ARMTargetMachine.cpp#L254), > > [3](https://github.com/llvm/llvm-project/blob/aa8f5e6156821aec1fed595f2e13d69655ec3311/llvm/lib/Target/X86/X86TargetMachine.cpp#L237). Yeah I get that we haven't been warning about a similar case (`--trap-unreachable`) before. But I think they are more of what ended up happen, and not the firm intention not to warn for conflicted requests. For example, that `--trap-unreachable` command line option was added much later (https://github.com/llvm/llvm-project/commit/6d9f8c98172cd4d648e33b21679325227c5cec83) than we set it to false (https://github.com/llvm/llvm-project/commit/ffa143ce814101fb1277ba65b20bdf86775d0b32). When first we set it to false in 2015, that option didn't exist so we had nothing to warn against. And someone who added that option in 2018 didn't go through all the code to check if there are any conflicting assignments. https://github.com/llvm/llvm-project/pull/65876 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
