- User Since
- Jan 7 2013, 9:35 AM (471 w, 2 d)
Fri, Jan 14
Thanks for fixing this.
Tue, Jan 4
Dec 16 2021
Dec 2 2021
Nov 8 2021
Nov 4 2021
LGTM. Let's get this in to fix https://bugs.chromium.org/p/chromium/issues/detail?id=1251909 and then we can investigate the Binaryen bug (and consider whether this or another approach is actually the best for these stackified local.gets) separately.
Nov 1 2021
Are there any type checker tests that are expected to fail?
Oct 29 2021
Oct 28 2021
it would be really cool if we could get ThinLTO working. It has most of the benefits of LTO at a small fraction of the cost.
Oct 27 2021
I didn't verify that the disassembly actually matches the body that we had before, but this looks good.
Oct 15 2021
Oct 7 2021
Oct 4 2021
It looks like this error is intended to catch mismatches for attributes that can affect codegen such as noreturn (in which case it makes sense to have it as an error) but it also now fires for cases such as __attribute__(warning()) which often do not duplicate the attribute on the definition. Is that intended?
Sep 29 2021
Sep 13 2021
I think this makes sense as a simple tweak to our current protocol. I could imagine us wanting to rethink it more generally in the future, but I don't think that needs to block the worklet fix.
Maybe also reference the bug number in the commit message?
Sep 10 2021
Sep 9 2021
I think the code is looking good.
I do think it will be cleaner in the long run to make the dylink section properly subsectioned, rather than the backwards-compatible version you have here. We've never declared any kind of backwords compat yet for dylibs, have we?
Sep 8 2021
LGTM, for the record.
Sep 7 2021
Sep 3 2021
Sep 2 2021
@aeubanks is probably the pass manager expert, and my knowledge is very likely out of date.
Does this actually work? I thought there was no such thing as a MachineModulePass and that backend passes couldn't be module passes. I wonder if that has changed.
Sep 1 2021
AFAIK we don't have gtest tests for wasm's lib/Object code so I don't want to block this little fix on creating a whole new set of tests. This change to the existing tests looks ok to me.
But I am interested in gtest unit tests more, because I like that style of testing generally. e.g. gor the file-based tests we have YAML as a convenient way to create test input, but it seems like for gtest tests you'd have to duplicate or write some custom test-harness code to generate inputs. Do you know if there are existing unit tests for lib/Object code, or know of any best practices for testing that library?
Aug 31 2021
I guess another option on the test would be to just keep the same sections that are in this test now, but still add foo at the front. The existing tests can stay the same and we could add one that covers this case (or we could switch one of the existing tests to use foo instead of producers if that would cover both cases, so we don't need to increase the total number of tests).
Aug 30 2021
Haven't looked at the code yet, but I'm a bit confused about cases 3/4.
If we expect a possible exception, but get a longjmp instead, why do we want to resume an exception rather than resuming the longjmp that we just caught? Likewise the other way around.
Aug 25 2021
Aug 24 2021
Aug 23 2021
looks like a nice simplification.
It might be worth ccing the author of SSAUpdaterBulk as an FYI
Aug 19 2021
Aug 18 2021
Aug 13 2021
when is Func->isInterposable() true? Is it just for weakly-defined functions (unlike e.g. in ELF where it's true anytime it's not DSO-local?)
OK, yeah that sounds good to me.
Abandoning in favor of D107940
Hmm, I've been thinking about this and I don't quite like the current candidates I proposed either. For example, what if someone wants to disable exceptions, i.e., lower all invokes to calls, but want to handle SjLj using Wasm EH instructions? The current options don't allow that, because -wasm-enable-sjlj requires -exception-model=wasm, and -exception-model=wasm means we enable Wasm EH.
Aug 12 2021
I get that enabling the new SjLj will require enabling the wasm EH feature, but it's not obvious why it should require --exception-model=wasm Is it possible to enable the new SJLJ with exception-model=none and have it force on -mattr=+exception-handling (or failing that, require only -mattr=+exception-handling and not both)? I would think e.g. this should eventually be the default for plain C code.
Actually wait, doesn't report_fatal_error act like a crash, complete with a stacktrace now?
Aug 5 2021
Aug 4 2021
LGTM too FTR
Jul 27 2021
I'd say from the llvm-objdump perspective (which only thinks of sections and not anything more finegrained other than instructions), anything other than the section header would be payload, including subsections etc. Unless we want llvm-objdump to be even smarter, a la wasm-objdump. I'm not sure if there's precedent for that in other formats or not.
but anyway this change is fine.
I guess one advantage of using objdump over obj2yaml when they are equivalent might be if we can get the whole test in one tool invocation instead of 2.
0000 01004180 080b1003 00000004 0000002a ..A............* 0010 0000002b 000000The problem here is that the section header is also displayed not just the actual data, so its less clear I think.
I guess depending on what the test is looking for, objdump might also be able to replace obj2yaml for non-code-section stuff too. At least data section maybe?
Jul 26 2021
Jul 23 2021
Jul 21 2021
Jul 19 2021
Jul 16 2021
- review comments
Jul 15 2021
This is now almost ready, just a couple of issues. Primarily, as the comment on line 510 of WasmObjectWriter.cpp suggests, I don't fully understand how the existing codepath (which uses the section symbol) works. I'm not sure if I should try to understand that more or not. And secondly I figured out a way to test the new behavior (but not the existing one) with a .s file, so I'm not sure whether we should keep the .ll test or not (it would catch any changes in how the dwarf code generates this MC, but I'm not sure we care about that, if the MC code is correct).
nice, first win for the type checker :)
Jul 14 2021
Jul 13 2021
- address more comments
Jul 12 2021
I went over all the tests and I think I've addressed all the issues.
- review round 3