sbc100 (Sam Clegg)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 16 2016, 10:22 AM (64 w, 4 d)

Recent Activity

Yesterday

sbc100 added a comment to D40759: [WebAssembly] Implement @llvm.global_ctors and @llvm.global_dtors.

How would you feel about landing a simplified version of this change that doesn't use the start section (i.e. always put start functions in the metadata)? Seems a little strange to special case the single start function. Also, I like the idea of letting the linker decide if/when to switch to using the start section (for now anyway).

Mon, Dec 11, 5:13 PM
sbc100 added a comment to D40875: [WebAssembly] Import the memory and table for .o files.

Re-landed as rL320432

Mon, Dec 11, 3:05 PM
sbc100 added a comment to D40844: Wasm COMDAT: core LLVM support.

Based on the discussion in the bug (https://bugs.llvm.org/show_bug.cgi?id=35533) it sounds like we need to take a look at SHF_LINK_ORDER and how that works. It sounds like its different to the "group" concept. But if we do decide groups are the way to go this change lgtm.

Mon, Dec 11, 2:36 PM
sbc100 updated the summary of D41093: [WebAssembly] check more details in call-indirect test.
Mon, Dec 11, 2:09 PM
sbc100 updated the summary of D41093: [WebAssembly] check more details in call-indirect test.
Mon, Dec 11, 2:08 PM
sbc100 created D41093: [WebAssembly] check more details in call-indirect test.
Mon, Dec 11, 2:07 PM
sbc100 updated the diff for D41038: [WebAssembly] Preserve ordering of global symbols.
  • feedback
Mon, Dec 11, 1:51 PM
sbc100 added a comment to D41038: [WebAssembly] Preserve ordering of global symbols.
In D41038#950727, @ncw wrote:

Looks good. Is the ordering mainly for the stability of the test output, or is it expected to have a tiny improvement to code locality at runtime?

There might be a couple of other places where we could do the same change. For example calculateImports has exactly the same iteration pattern, where the imports are ordered according to the first-importing file, rather than ordered by the file that actually uses the import. Harder to see how to fix that one though. I wouldn't have thought it's worth trying.

Mon, Dec 11, 11:58 AM

Fri, Dec 8

sbc100 abandoned D37831: [WebAssembly] Don't pass -ffunction-section/-fdata-sections by default.
Fri, Dec 8, 4:03 PM
sbc100 updated the diff for D41033: Use ErrorOS for log messages as well as error.
  • update tests
Fri, Dec 8, 3:45 PM
sbc100 updated the diff for D41033: Use ErrorOS for log messages as well as error.
  • feedback
Fri, Dec 8, 3:24 PM
sbc100 added a reviewer for D41033: Use ErrorOS for log messages as well as error: ruiu.
Fri, Dec 8, 3:24 PM
sbc100 added reviewers for D41038: [WebAssembly] Preserve ordering of global symbols: ruiu, ncw.
Fri, Dec 8, 3:22 PM
sbc100 created D41038: [WebAssembly] Preserve ordering of global symbols.
Fri, Dec 8, 3:22 PM
sbc100 updated the diff for D40989: [WebAssembly] De-dup indirect function table.
  • remove unused member
Fri, Dec 8, 3:15 PM
sbc100 updated the summary of D41033: Use ErrorOS for log messages as well as error.
Fri, Dec 8, 2:16 PM
sbc100 updated the diff for D41033: Use ErrorOS for log messages as well as error.
  • cleanup
Fri, Dec 8, 2:16 PM
sbc100 created D41033: Use ErrorOS for log messages as well as error.
Fri, Dec 8, 2:14 PM
sbc100 added a comment to D41024: [WebAssembly] Improve wasm test cases.
In D41024#949863, @ruiu wrote:

LGTM, but please submit this as two separate patches -- one for weak symbols and the other to fix -mtriple.

Fri, Dec 8, 11:29 AM
sbc100 added a comment to D40875: [WebAssembly] Import the memory and table for .o files.

This can be re-landed one this lld change lands: https://reviews.llvm.org/D40989

Fri, Dec 8, 10:52 AM
sbc100 updated the diff for D40989: [WebAssembly] De-dup indirect function table.
  • rebase
Fri, Dec 8, 10:49 AM
sbc100 retitled D41024: [WebAssembly] Improve wasm test cases from [WebAssembly] Add test for weakly defined symbols to [WebAssembly] Improve wasm test cases.
Fri, Dec 8, 10:34 AM
sbc100 updated the diff for D41024: [WebAssembly] Improve wasm test cases.
  • call indirect test
Fri, Dec 8, 10:31 AM
sbc100 added a comment to D40989: [WebAssembly] De-dup indirect function table.

Good call regarding that test, I added it here in a separate change so we can see how this change effects it:
https://reviews.llvm.org/D41024

Fri, Dec 8, 10:28 AM
sbc100 created D41024: [WebAssembly] Improve wasm test cases.
Fri, Dec 8, 10:27 AM

Thu, Dec 7

sbc100 added a comment to D40845: Wasm COMDAT: LLD support.

I create a patch based partially on this one that just does the compression of the function table. https://reviews.llvm.org/D40989.
Hopefully that means this can be rebased into a smaller change.

Thu, Dec 7, 5:13 PM
sbc100 added inline comments to D40725: Wasm entrypoint changes #3 (add --no-entry argument to LLD).
Thu, Dec 7, 5:09 PM
sbc100 added a reviewer for D40993: Prefer `ArrayRef` over `const std::vector&`: ruiu.
Thu, Dec 7, 5:01 PM
sbc100 updated the diff for D40989: [WebAssembly] De-dup indirect function table.
  • feedback
Thu, Dec 7, 5:01 PM
sbc100 created D40993: Prefer `ArrayRef` over `const std::vector&`.
Thu, Dec 7, 5:01 PM
sbc100 retitled D40990: [WebAssembly] Add check to ensure symbol VA is only set once. NFC from [WebAssembly] Add check to ensure symbol VA is only set once to [WebAssembly] Add check to ensure symbol VA is only set once. NFC.
Thu, Dec 7, 4:07 PM
sbc100 added a reviewer for D40990: [WebAssembly] Add check to ensure symbol VA is only set once. NFC: ruiu.
Thu, Dec 7, 4:07 PM
sbc100 created D40990: [WebAssembly] Add check to ensure symbol VA is only set once. NFC.
Thu, Dec 7, 4:06 PM
sbc100 updated the diff for D40989: [WebAssembly] De-dup indirect function table.
  • remove comment
Thu, Dec 7, 4:00 PM
sbc100 added reviewers for D40989: [WebAssembly] De-dup indirect function table: ruiu, ncw.
Thu, Dec 7, 3:56 PM
sbc100 created D40989: [WebAssembly] De-dup indirect function table.
Thu, Dec 7, 3:54 PM
sbc100 abandoned D40760: [WebAssembly] Add support for __init_array_start and friends.
Thu, Dec 7, 10:35 AM
sbc100 added a comment to D40760: [WebAssembly] Add support for __init_array_start and friends.

I think I'm going to abandon this, but the work in this patch and in Nicolas's patch will be probably still end up being generally useful.

Thu, Dec 7, 10:35 AM

Wed, Dec 6

sbc100 abandoned D40669: Revert "[WebAssembly] Update test expectations for gcc torture tests".
Wed, Dec 6, 7:23 PM
sbc100 added a comment to D39865: Use default member initialization where possible.

Manually. Is there clang-tidy method of doing this? This change is pretty conservative, it only includes to more obvious cases I could find.

Wed, Dec 6, 5:53 PM
sbc100 added a comment to D40859: [WebAssembly] Export globals when linking with -r/--relocatable.

Sorry, didn't mean to imply you were going slow. I just wasn't sure if I was waiting on you at all in this case since I got another LGTM. I can wait :)

Wed, Dec 6, 5:25 PM
sbc100 added a comment to D40859: [WebAssembly] Export globals when linking with -r/--relocatable.

In case it wasn't clear I was asking about this change in particular too. Want me to wait on your LG?

Wed, Dec 6, 5:18 PM
sbc100 added a comment to D40875: [WebAssembly] Import the memory and table for .o files.

Actually if you don't mind I'd like to revert this and prepare a nicer lld-side fix that will work both before and after this change. WDYT?

Wed, Dec 6, 5:05 PM
sbc100 added a comment to D40875: [WebAssembly] Import the memory and table for .o files.

Looks like this requires a corresponding lld change, broke some tests because we were looking the ->tables() on the wasm objects (which is now empty, since they are in the ->imports() now). I'm looking into a few different ways to fix.

Wed, Dec 6, 5:04 PM
sbc100 added a comment to D40859: [WebAssembly] Export globals when linking with -r/--relocatable.

@ruiu, do you prefer that I wait for your LGTM on all lld changes? I'm assuming you would be ok with small-ish wasm-only changes being peer-reviewed rather than requiring owner reviewed? e(assuming you consider yourself the owner of lld?)

Wed, Dec 6, 4:59 PM
sbc100 added a comment to D40906: Wasm: section kind can be code.

I'm gonna see if I can come up with precise test for this, and then will land it.

Wed, Dec 6, 4:40 PM
sbc100 added a comment to D40760: [WebAssembly] Add support for __init_array_start and friends.

I'm not quite ready to land this anyway. It needs rebasing and integrating with @ncw proposed version.

Wed, Dec 6, 3:47 PM
sbc100 accepted D40875: [WebAssembly] Import the memory and table for .o files.
Wed, Dec 6, 3:20 PM
sbc100 added a comment to D40875: [WebAssembly] Import the memory and table for .o files.

Can you update the description.

Wed, Dec 6, 2:31 PM
sbc100 added a comment to D40559: Wasm entrypoint changes #2 (export entrypoint in "start" section) APPLY AFTER D40724.

I'd rather not change the behaviour of --entry since its a flag that is in common usage. I'd also rather not default --no-entry since that means that by default nothing will get pulled into the link (i.e. all users will need explicit --entry and/or --undefined otherwise nothing will be linked).

Wed, Dec 6, 12:59 PM
sbc100 added a comment to D40906: Wasm: section kind can be code.

I wonder if we can add test for this? I guess it doesn't effect the -elf target only the -wasm one?

Wed, Dec 6, 12:15 PM
sbc100 accepted D40906: Wasm: section kind can be code.
Wed, Dec 6, 12:14 PM
sbc100 added inline comments to D40906: Wasm: section kind can be code.
Wed, Dec 6, 11:25 AM

Tue, Dec 5

sbc100 added a comment to D40559: Wasm entrypoint changes #2 (export entrypoint in "start" section) APPLY AFTER D40724.

Sadly I think we are blocked on: https://github.com/WebAssembly/design/issues/1160.

Tue, Dec 5, 7:27 PM
sbc100 accepted D40725: Wasm entrypoint changes #3 (add --no-entry argument to LLD).

LGTM - with a couple more comments.

Tue, Dec 5, 7:22 PM
sbc100 added a comment to D40760: [WebAssembly] Add support for __init_array_start and friends.

Actually they are weakly referenced in musl:
https://github.com/jfbastien/musl/blob/wasm-prototype-1/src/env/__libc_start_main.c#L14

Tue, Dec 5, 5:28 PM
sbc100 retitled D40870: [WebAssembly] Remove wasm/Strings.cpp+h. NFC from [WebAssembly] Remove wasm/Strings.cpp+h to [WebAssembly] Remove wasm/Strings.cpp+h. NFC.
Tue, Dec 5, 4:31 PM
sbc100 retitled D40870: [WebAssembly] Remove wasm/Strings.cpp+h. NFC from [WebAssembly] Remove wasm/Strings.cpp/wasm/Strings.h to [WebAssembly] Remove wasm/Strings.cpp+h.
Tue, Dec 5, 4:31 PM
sbc100 created D40870: [WebAssembly] Remove wasm/Strings.cpp+h. NFC.
Tue, Dec 5, 4:17 PM
sbc100 updated the diff for D40866: [docs] Formatting-only change.
  • revert
Tue, Dec 5, 3:28 PM
sbc100 created D40866: [docs] Formatting-only change.
Tue, Dec 5, 3:26 PM
sbc100 added a comment to D40859: [WebAssembly] Export globals when linking with -r/--relocatable.

@ncw , this is change I've be wroking on for a while now that might be help (or an annoying merge conflict!) for your work on function stripping. It basically does for globals what I imagine we would want to do for functions (i.e. eliminates the per file index offset). Sorry if it doesn't merge well with your work, but hopefully there are things that they share and could make your partch smaller?

Tue, Dec 5, 2:58 PM
sbc100 added reviewers for D40859: [WebAssembly] Export globals when linking with -r/--relocatable: ruiu, ncw.
Tue, Dec 5, 2:55 PM
sbc100 retitled D40859: [WebAssembly] Export globals when linking with -r/--relocatable from [WebAssembly] Fix symbol exports under -r/--relocatable to [WebAssembly] Export globals when linking with -r/--relocatable.
Tue, Dec 5, 2:54 PM
sbc100 updated the summary of D40859: [WebAssembly] Export globals when linking with -r/--relocatable.
Tue, Dec 5, 2:53 PM
sbc100 updated the summary of D40859: [WebAssembly] Export globals when linking with -r/--relocatable.
Tue, Dec 5, 2:51 PM
sbc100 created D40859: [WebAssembly] Export globals when linking with -r/--relocatable.
Tue, Dec 5, 2:46 PM
sbc100 added a comment to D39984: [docs] Update doc build instructions and some reformatting.

ping. The sphinx_intro.rst changes are actually real (not just reformatting). I updated them to the cmake world.

Tue, Dec 5, 2:20 PM
sbc100 accepted D40724: Wasm entrypoint changes #1 (add --undefined argument to LLD).

Thanks. LGTM

Tue, Dec 5, 11:54 AM
sbc100 added a reviewer for D40847: [WebAssembly] Fix stack pointer relocations: ruiu.
Tue, Dec 5, 11:43 AM
sbc100 created D40847: [WebAssembly] Fix stack pointer relocations.
Tue, Dec 5, 11:42 AM
sbc100 updated the diff for D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC.
  • Inline kStackPointer
Tue, Dec 5, 11:03 AM
sbc100 added a comment to D40844: Wasm COMDAT: core LLVM support.

Thanks for doing this. Looks like a good direction!

Tue, Dec 5, 11:01 AM
sbc100 updated the diff for D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC.
  • Review feedback
Tue, Dec 5, 10:37 AM
sbc100 updated the diff for D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC.
  • define kStackPointer inside namespace
Tue, Dec 5, 10:32 AM
sbc100 updated subscribers of D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC.
Tue, Dec 5, 10:31 AM
sbc100 added inline comments to D40760: [WebAssembly] Add support for __init_array_start and friends.
Tue, Dec 5, 10:30 AM
sbc100 added a comment to D40760: [WebAssembly] Add support for __init_array_start and friends.
In D40760#945281, @ncw wrote:

I believe __init_aray_start is weakly referenced by libc.. so when its not defined it ends up being null, which means it will not iterate over any initializes. If this wasn't true we'd be seeing link failures with undefined symbols in the tests we are doing today.

Are you sure? I don't mean to contradict, but with a master build, I find that undefined weak symbols are set to (uint32_t)-1 when not found, and secondly that the output has a global import for __init_array_start if I link in anything from libc that uses it.

My test:

// File test.c
int main() { return 0; }

Commands:

clang -o /tmp/test.o -c /tmp/test.c
wasm-ld -L/home/ncw/workspace/llvm/compile-root/usr/lib /tmp/test.o /home/ncw/workspace/llvm/compile-root/usr/lib/crt1.o -lc -o /tmp/test --allow-undefined

Output contains:

(import "env" "__syscall_set_tid_address" (func $import$0 (param i32 i32) (result i32)))
(import "env" "__syscall_poll" (func $import$1 (param i32 i32) (result i32)))
(import "env" "__syscall_open" (func $import$2 (param i32 i32) (result i32)))
(import "env" "__syscall_exit_group" (func $import$3 (param i32 i32) (result i32)))
(import "env" "__syscall_exit" (func $import$4 (param i32 i32) (result i32)))
(import "env" "__init_array_start" (global $import$5 i32))
(import "env" "__init_array_end" (global $import$6 i32))
Tue, Dec 5, 10:29 AM
sbc100 added a reviewer for D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC: ruiu.
Tue, Dec 5, 10:19 AM
sbc100 retitled D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC from [WebAssembly] Improve support linker synthetic symbols. NFC to [WebAssembly] Improve support for linker synthetic symbols. NFC.
Tue, Dec 5, 10:19 AM
sbc100 retitled D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC from [WebAssembly] Improve support linker synthetic symbols to [WebAssembly] Improve support linker synthetic symbols. NFC.
Tue, Dec 5, 10:19 AM
sbc100 updated the summary of D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC.
Tue, Dec 5, 10:18 AM
sbc100 updated the summary of D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC.
Tue, Dec 5, 10:17 AM
sbc100 created D40843: [WebAssembly] Improve support for linker synthetic symbols. NFC.
Tue, Dec 5, 10:17 AM

Mon, Dec 4

sbc100 added a reviewer for D40825: [WebAssembly] Simplify check for emitting relocations: ruiu.
Mon, Dec 4, 8:46 PM
sbc100 retitled D40825: [WebAssembly] Simplify check for emitting relocations from [WebAssembly] Simplify check for emitting relocations This is a small change I split of from a larger one that simplifies the condition that need to be checked when decided if we need to emit relocation and all the things they depend on (symbols... to [WebAssembly] Simplify check for emitting relocations.
Mon, Dec 4, 8:45 PM
sbc100 added inline comments to D40824: toString function take a const refs where possible.
Mon, Dec 4, 8:44 PM
sbc100 created D40825: [WebAssembly] Simplify check for emitting relocations.
Mon, Dec 4, 8:41 PM
sbc100 updated the diff for D40824: toString function take a const refs where possible.
  • revert part
Mon, Dec 4, 8:26 PM
sbc100 added a reviewer for D40824: toString function take a const refs where possible: ruiu.
Mon, Dec 4, 8:25 PM
sbc100 created D40824: toString function take a const refs where possible.
Mon, Dec 4, 8:24 PM

Sun, Dec 3

sbc100 added a comment to D40771: [WebAssembly] Remove used --sysroot option.

I would argue that we can always add this feature if the future, but implementing now seems unnecessary.

Sun, Dec 3, 9:21 AM
sbc100 added a comment to D40771: [WebAssembly] Remove used --sysroot option.

It allows one to write:

Sun, Dec 3, 9:20 AM
sbc100 added a comment to D40771: [WebAssembly] Remove used --sysroot option.

I'm not removing --sysroot from the toolchain (i.e. clang and the driver), only from lld. In lld only the ELF driver supports sysroot and the only thing is provides is that ability for the user to pass special -L flag with = at the start. i.e. -L=/path/in/sysroot. Do you think this is useful feature? Normally the sysroot library paths are constructed by clang based on the --sysroot you pass to it, and are fully expanded by the time the linker sees them.

Sun, Dec 3, 9:18 AM

Sat, Dec 2

sbc100 added a reviewer for D40773: Reland "[WebAssembly] Add support for visibility flag"": aheejin.
Sat, Dec 2, 5:47 PM
sbc100 created D40773: Reland "[WebAssembly] Add support for visibility flag"".
Sat, Dec 2, 5:47 PM
sbc100 updated subscribers of D40772: Reland "[WebAssembly] Add visibility flag to Wasm symbol flags"".
Sat, Dec 2, 5:19 PM
sbc100 added a reviewer for D40772: Reland "[WebAssembly] Add visibility flag to Wasm symbol flags"": aheejin.
Sat, Dec 2, 5:17 PM
sbc100 created D40772: Reland "[WebAssembly] Add visibility flag to Wasm symbol flags"".
Sat, Dec 2, 5:16 PM
sbc100 updated the diff for D40771: [WebAssembly] Remove used --sysroot option.
  • remove from .td file
Sat, Dec 2, 3:44 PM