Page MenuHomePhabricator

sbc100 (Sam Clegg)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 16 2016, 10:22 AM (214 w, 2 d)

Recent Activity

Fri, Oct 23

sbc100 committed rG69e2797eaed5: [WebAssembly] Implementation of (most) table instructions (authored by pmatos).
[WebAssembly] Implementation of (most) table instructions
Fri, Oct 23, 8:43 AM
sbc100 closed D89797: [WebAssembly] Implementation of (most) table instructions.
Fri, Oct 23, 8:43 AM · Restricted Project

Thu, Oct 22

sbc100 added a comment to D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library..

I really do think this needs to be under object. It can be separate in there as lib/Object/Copy if you want, but I don't think it should be a parallel directory. Hopefully the updates for the move aren't overly onerous here.

-eric

Won't that mean that any tool that just wants to read in object files (e.g. lld) will need to build this extra code for no reason? Not a huge deal I guess (especially for lld which due to LTO pulls in all of llvm!) but I imagine there are many tools that just want to read (and not modify) objects.

That will depend on the archiver/linker behaviour, I expect, but at least for standard ld.lld behaviour (I can't speak for others), only the objects that actually are needed are pulled from the archive. Therefore, for example, if LLD didn't reference anything in the objcopy codebase, it wouldn't actually use those archive members, and therefore it wouldn't be added to the tools code. Even if it did, linker options like --gc-sections or equivalent likely would cause the unused code to be removed at link time, if the tool is built appropriately.

Thu, Oct 22, 2:03 PM · Restricted Project
sbc100 added a comment to D89797: [WebAssembly] Implementation of (most) table instructions.

This mostly lgtm. I would love it if we could avoid adding the new MVT value for now since then we could avoid changing the shared code on llvm/lib/CodeGen/ValueTypes.cpp.. but perhaps that absolutely needed for this change?

Thu, Oct 22, 11:06 AM · Restricted Project

Wed, Oct 21

sbc100 added a comment to D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library..

I really do think this needs to be under object. It can be separate in there as lib/Object/Copy if you want, but I don't think it should be a parallel directory. Hopefully the updates for the move aren't overly onerous here.

-eric

Wed, Oct 21, 9:19 AM · Restricted Project
sbc100 added inline comments to D89797: [WebAssembly] Implementation of (most) table instructions.
Wed, Oct 21, 8:24 AM · Restricted Project
sbc100 added a comment to D89797: [WebAssembly] Implementation of (most) table instructions.

No I wouldn't format any code you didn't write.. nor assume that clang-format always knows best in all cases.

Wed, Oct 21, 8:19 AM · Restricted Project

Tue, Oct 20

sbc100 added a comment to D89797: [WebAssembly] Implementation of (most) table instructions.

In general I'm loving how small this change is. I don't really grok the tablegen stuff but everything else looks pretty good to me.

Tue, Oct 20, 3:28 PM · Restricted Project

Tue, Oct 13

sbc100 committed rG388fb67b0dd7: [WebAssembly] Added .tabletype to asm and multiple table support in obj files (authored by pmatos).
[WebAssembly] Added .tabletype to asm and multiple table support in obj files
Tue, Oct 13, 7:53 AM
sbc100 closed D88815: [WebAssembly] Added .tabletype to asm and multiple table support in obj files.
Tue, Oct 13, 7:52 AM · Restricted Project

Mon, Oct 12

sbc100 added a comment to D88815: [WebAssembly] Added .tabletype to asm and multiple table support in obj files.

Other than that this looks good to land

Mon, Oct 12, 9:55 PM · Restricted Project
sbc100 added a comment to D88815: [WebAssembly] Added .tabletype to asm and multiple table support in obj files.

I seeing:

Mon, Oct 12, 9:55 PM · Restricted Project
sbc100 committed rGb3b4cda10406: [lld][WebAssembly] Don't GC library objects under `--whole-archive` (authored by sbc100).
[lld][WebAssembly] Don't GC library objects under `--whole-archive`
Mon, Oct 12, 9:19 PM
sbc100 closed D89290: [lld][WebAssembly] Don't GC library objects under `--whole-archive`.
Mon, Oct 12, 9:19 PM · Restricted Project
sbc100 added a comment to D89290: [lld][WebAssembly] Don't GC library objects under `--whole-archive`.

I think mind is slightly less intrusive.. and I like having the single test. So I'll land this then.

Mon, Oct 12, 9:17 PM · Restricted Project
sbc100 added a comment to D89290: [lld][WebAssembly] Don't GC library objects under `--whole-archive`.

Looks like we raced to write this patch :)

Mon, Oct 12, 9:15 PM · Restricted Project
sbc100 added a comment to D89293: [WebAssembly] Don't GC constructors from libraries under --whole-archive.

Wow you are fast :) do you prefer this solution over mine?

Mon, Oct 12, 9:15 PM · Restricted Project
sbc100 added a comment to D85062: [WebAssembly] GC constructor functions in otherwise unused archive objects.

Fix (I believe) is in https://reviews.llvm.org/D89290

Mon, Oct 12, 8:48 PM · Restricted Project
sbc100 added a reviewer for D89290: [lld][WebAssembly] Don't GC library objects under `--whole-archive`: sunfish.
Mon, Oct 12, 8:47 PM · Restricted Project
sbc100 requested review of D89290: [lld][WebAssembly] Don't GC library objects under `--whole-archive`.
Mon, Oct 12, 8:47 PM · Restricted Project
sbc100 added a comment to D85062: [WebAssembly] GC constructor functions in otherwise unused archive objects.

Looks like we have a specific emscripten test for this since it started failing on our llvm roller:
https://github.com/emscripten-core/emscripten/blob/bd8e5b5296721e33cfad98322e6b70b9985cfd2d/tests/test_other.py#L900

Mon, Oct 12, 8:34 PM · Restricted Project
sbc100 committed rG2513407d3950: [lld][WebAssembly] Add support for -Bsymbolic flag (authored by sbc100).
[lld][WebAssembly] Add support for -Bsymbolic flag
Mon, Oct 12, 5:25 PM
sbc100 closed D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Mon, Oct 12, 5:25 PM · Restricted Project
sbc100 added inline comments to D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Mon, Oct 12, 5:23 PM · Restricted Project
sbc100 added inline comments to D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Mon, Oct 12, 5:15 PM · Restricted Project
sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.
  • typo
Mon, Oct 12, 4:59 PM · Restricted Project
sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.

comment

Mon, Oct 12, 4:58 PM · Restricted Project
sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.

bsymbol is the default baviour for executables

Mon, Oct 12, 4:56 PM · Restricted Project
sbc100 accepted D85062: [WebAssembly] GC constructor functions in otherwise unused archive objects.

lgtm % test nits

Mon, Oct 12, 3:04 PM · Restricted Project
sbc100 added a comment to D88815: [WebAssembly] Added .tabletype to asm and multiple table support in obj files.

It looks like there are a bunch of wasm-ld tests that need similar updates.

Mon, Oct 12, 1:59 PM · Restricted Project
sbc100 accepted D88815: [WebAssembly] Added .tabletype to asm and multiple table support in obj files.

Is this ready to land now?

Mon, Oct 12, 12:01 PM · Restricted Project
sbc100 accepted D88815: [WebAssembly] Added .tabletype to asm and multiple table support in obj files.
Mon, Oct 12, 8:35 AM · Restricted Project
sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.

Revert readobj part

Mon, Oct 12, 7:34 AM · Restricted Project
sbc100 added a comment to D89152: [lld][WebAssembly] Add support for -Bsymbolic.

sbc100 edited reviewers, ... removed: jhenderson.

(I've got a herald rule to auto-add myself to reviews touching llvm-readobj amongst other tools - no point trying to work around it!)

Sorry, didn't mean to try to work around your herald stuff, I also use herald to be notified about WebAssembly related thing. I think I was just trying to be precise about who I thought would be good reviewer. I won't remove your name anymore from that field.

In general, would it would make sense to add yourself to subscriber rather than reviewer if you want to monitor all changes in these areas? You could then add yourself as a reviewer to change that you care about? Or do you want to actually be a reviewer all of all change to lld? For example, this specific change, do you want to be a reviewer?

Mon, Oct 12, 6:57 AM · Restricted Project
sbc100 added inline comments to D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Mon, Oct 12, 6:56 AM · Restricted Project
sbc100 added a comment to D89152: [lld][WebAssembly] Add support for -Bsymbolic.

sbc100 edited reviewers, ... removed: jhenderson.

(I've got a herald rule to auto-add myself to reviews touching llvm-readobj amongst other tools - no point trying to work around it!)

Mon, Oct 12, 6:54 AM · Restricted Project

Fri, Oct 9

sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.

revert

Fri, Oct 9, 1:51 PM · Restricted Project
sbc100 updated the summary of D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Fri, Oct 9, 12:48 PM · Restricted Project
sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.

feedback

Fri, Oct 9, 12:30 PM · Restricted Project
sbc100 added inline comments to D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Fri, Oct 9, 11:56 AM · Restricted Project
sbc100 added inline comments to D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Fri, Oct 9, 11:49 AM · Restricted Project
sbc100 added a comment to D89152: [lld][WebAssembly] Add support for -Bsymbolic.

This change will allows us to remove the corresponding binaryen code: https://github.com/WebAssembly/binaryen/pull/3211 and instead pass -Bsymbolic when linking MAIN_MODULE's in emscripten.

Fri, Oct 9, 11:36 AM · Restricted Project
sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.

comments

Fri, Oct 9, 11:19 AM · Restricted Project
sbc100 updated the diff for D89152: [lld][WebAssembly] Add support for -Bsymbolic.

comments

Fri, Oct 9, 11:14 AM · Restricted Project
sbc100 edited reviewers for D89152: [lld][WebAssembly] Add support for -Bsymbolic, added: azakai, dschuff; removed: jhenderson.
Fri, Oct 9, 11:11 AM · Restricted Project
sbc100 requested review of D89152: [lld][WebAssembly] Add support for -Bsymbolic.
Fri, Oct 9, 11:06 AM · Restricted Project

Wed, Oct 7

sbc100 accepted D88815: [WebAssembly] Added .tabletype to asm and multiple table support in obj files.

Looks great! Just a couple of nits

Wed, Oct 7, 1:30 PM · Restricted Project

Tue, Oct 6

sbc100 accepted D88697: [WebAssembly] Rename Emscripten EH functions.
Tue, Oct 6, 8:07 AM · Restricted Project

Mon, Oct 5

sbc100 added inline comments to D88697: [WebAssembly] Rename Emscripten EH functions.
Mon, Oct 5, 8:02 PM · Restricted Project

Fri, Oct 2

sbc100 added a comment to D88697: [WebAssembly] Rename Emscripten EH functions.

In PR description: final wasm times -> but final wasm types

Fri, Oct 2, 2:27 PM · Restricted Project

Wed, Sep 30

sbc100 committed rG3c45a06f26ed: [lld][WebAssembly] Allow exporting of mutable globals (authored by sbc100).
[lld][WebAssembly] Allow exporting of mutable globals
Wed, Sep 30, 5:54 PM
sbc100 closed D88506: [lld][WebAssembly] Allow exporting of mutable globals.
Wed, Sep 30, 5:54 PM · Restricted Project
sbc100 updated the diff for D88506: [lld][WebAssembly] Allow exporting of mutable globals.

docs

Wed, Sep 30, 5:52 PM · Restricted Project
sbc100 accepted D81689: [WebAssembly] New-style command support.

The description needs updating to reflect the fact that this change no longer adds the --emscripten flag.

Wed, Sep 30, 5:17 PM · Restricted Project
sbc100 accepted D88603: [WebAssembly] Add support for DWARF type units.
Wed, Sep 30, 12:07 PM · Restricted Project, Restricted Project

Tue, Sep 29

sbc100 added a comment to D88506: [lld][WebAssembly] Allow exporting of mutable globals.

I think it's kind of confusing that export-all doesn't actually export all the symbols. What if we instead made export-all exactly equivalent to individually exporting all the symbols so that it errors out if mutable-globals is not enabled and any of the symbols is a mutable global? Then we could add a --no-export=<symbol> flag that users or drivers could use to filter out the mutable global symbols if they can't enable mutable-globals and need to avoid the error.

Tue, Sep 29, 1:28 PM · Restricted Project
sbc100 updated the summary of D88506: [lld][WebAssembly] Allow exporting of mutable globals.
Tue, Sep 29, 11:14 AM · Restricted Project
sbc100 updated the diff for D88506: [lld][WebAssembly] Allow exporting of mutable globals.

Typeos

Tue, Sep 29, 11:13 AM · Restricted Project
sbc100 added reviewers for D88506: [lld][WebAssembly] Allow exporting of mutable globals: tlively, RReverser.
Tue, Sep 29, 11:11 AM · Restricted Project
sbc100 requested review of D88506: [lld][WebAssembly] Allow exporting of mutable globals.
Tue, Sep 29, 11:11 AM · Restricted Project

Mon, Sep 28

sbc100 updated subscribers of D87258: [WebAssembly, LowerTypeTests] Fix control-flow integrity support.
Mon, Sep 28, 10:07 AM · Restricted Project
sbc100 accepted D88428: [WebAssembly] Use wasm::Signature for in ObjectWriter (NFC).

I think this is probably just historical. llvm/Object/WasmTraits.h is relatively new I think. My original plan was to keep the public interface to llvm/Object/ very dumb without any classes or methods, but I think it has evolved into something a little more rich now.

Mon, Sep 28, 9:43 AM · Restricted Project

Sat, Sep 26

sbc100 added a comment to D88369: [lld][WebAssembly] Fix segfault in map file support.

Can you update the mapfile test to tickle this?

Sat, Sep 26, 4:55 PM · Restricted Project

Sep 24 2020

sbc100 accepted D79530: [lld][WebAssembly] Allow `atomics` feature with unshared memory.

But I guess its only temporary as soon engines will catch up and the error will go away.

Sep 24 2020, 3:14 PM · Restricted Project
sbc100 added a comment to D79530: [lld][WebAssembly] Allow `atomics` feature with unshared memory.

Imagine a project that links statically without threads but its as dependency on some autoconf or cmake library that happens to throw in the -pthread... such users would hit this error today so I guess its only new users who do this that would be a runtime rather than link time error after this change.

Sep 24 2020, 3:13 PM · Restricted Project
sbc100 accepted D88262: [WebAssembly] Make SjLj lowering globals thread-local.

Awesome thanks!

Sep 24 2020, 2:47 PM · Restricted Project
sbc100 added a comment to D88226: Add debug option to wasm-ld to enable LLVM style debug info.

This is already supported like via -mllvm. E.g. wasm-ld -mllvm -debug or wasm-ld -mllvm -debug-only=lld

Sep 24 2020, 8:12 AM · Restricted Project

Sep 21 2020

sbc100 added a comment to D87407: [WebAssembly][MC] Fix computation of relative symbol offset.

Do you have commit access or should I land this for you?

Sep 21 2020, 9:46 PM · Restricted Project
sbc100 accepted D87407: [WebAssembly][MC] Fix computation of relative symbol offset.

Great!

Sep 21 2020, 9:39 PM · Restricted Project
sbc100 added a comment to D87407: [WebAssembly][MC] Fix computation of relative symbol offset.

Thanks for fixing this! Sorry it took me so long to get back to you. This lgtm % some comments.

Sep 21 2020, 4:52 PM · Restricted Project

Sep 16 2020

sbc100 accepted D85685: Support dwarf fission for wasm object files.

I don't really grok the TargetFrameLowering::DwarfFrameBase part but everything else LGTM

Sep 16 2020, 3:42 PM · Restricted Project, Restricted Project
sbc100 added a comment to D31413: [libc++] Use __attribute__((init_priority(101))) to ensure streams get initialized early.

Might even be worth backporting such as simple but useful fix to the 11 release?

Sep 16 2020, 9:37 AM · Restricted Project
sbc100 accepted D31413: [libc++] Use __attribute__((init_priority(101))) to ensure streams get initialized early.

I'd love to see this land so we can drop our downstream patch in emscripten and also fix the outstanding wasi-sdk issue.

Sep 16 2020, 9:36 AM · Restricted Project

Sep 15 2020

sbc100 added a comment to D85685: Support dwarf fission for wasm object files.

Seems reasonable. Do you think this way is cleaner than the way elf does it? Looks like ELF creates two different ELFWriter inside the ELFDwoObjectWriter subclass right?

Sep 15 2020, 6:16 PM · Restricted Project, Restricted Project
sbc100 added a comment to D31413: [libc++] Use __attribute__((init_priority(101))) to ensure streams get initialized early.

This came up again in wasi-sdk: https://github.com/WebAssembly/wasi-sdk/issues/153

Sep 15 2020, 5:58 PM · Restricted Project
Herald added a reviewer for D74885: [libcxx] Construct __start_std_streams at high init priority: Restricted Project.

This came up again in wasi-sdk: https://github.com/WebAssembly/wasi-sdk/issues/153

Sep 15 2020, 5:58 PM · Restricted Project
sbc100 committed rG3f411e97739f: [lld][WebAssembly] Fix --export-all when __stack_pointer is present (authored by sbc100).
[lld][WebAssembly] Fix --export-all when __stack_pointer is present
Sep 15 2020, 6:20 AM
sbc100 closed D87663: [lld][WebAssembly] Fix --export-all when __stack_pointer is present.
Sep 15 2020, 6:20 AM · Restricted Project
sbc100 added inline comments to D87663: [lld][WebAssembly] Fix --export-all when __stack_pointer is present.
Sep 15 2020, 6:19 AM · Restricted Project
sbc100 updated the diff for D87663: [lld][WebAssembly] Fix --export-all when __stack_pointer is present.

revert part

Sep 15 2020, 6:17 AM · Restricted Project
sbc100 added inline comments to D87663: [lld][WebAssembly] Fix --export-all when __stack_pointer is present.
Sep 15 2020, 5:41 AM · Restricted Project

Sep 14 2020

sbc100 committed rG2c12b056bece: [lld][WebAssembly] Allow globals imports via import_name/import_module (authored by sbc100).
[lld][WebAssembly] Allow globals imports via import_name/import_module
Sep 14 2020, 8:35 PM
sbc100 closed D87666: [lld][WebAssembly] Allow globals imports via import_name/import_module.
Sep 14 2020, 8:35 PM · Restricted Project
sbc100 updated the diff for D87666: [lld][WebAssembly] Allow globals imports via import_name/import_module.

feedback

Sep 14 2020, 8:34 PM · Restricted Project
sbc100 added a comment to D87666: [lld][WebAssembly] Allow globals imports via import_name/import_module.

Should F and G be lowercased? It looks like the surrounding code uses camelCase fairly uniformly.

Sep 14 2020, 8:34 PM · Restricted Project
sbc100 added a reviewer for D87666: [lld][WebAssembly] Allow globals imports via import_name/import_module: tlively.
Sep 14 2020, 7:21 PM · Restricted Project
sbc100 requested review of D87666: [lld][WebAssembly] Allow globals imports via import_name/import_module.
Sep 14 2020, 7:21 PM · Restricted Project
sbc100 updated the diff for D87663: [lld][WebAssembly] Fix --export-all when __stack_pointer is present.

fix

Sep 14 2020, 6:59 PM · Restricted Project
sbc100 added a reviewer for D87663: [lld][WebAssembly] Fix --export-all when __stack_pointer is present: tlively.
Sep 14 2020, 6:32 PM · Restricted Project
sbc100 requested review of D87663: [lld][WebAssembly] Fix --export-all when __stack_pointer is present.
Sep 14 2020, 6:32 PM · Restricted Project
sbc100 accepted D87659: clang-format.
Sep 14 2020, 6:17 PM · Restricted Project

Sep 12 2020

sbc100 committed rGcc2da5554b5e: [lld][WebAssembly] Add initial support for -Map/--print-map (authored by sbc100).
[lld][WebAssembly] Add initial support for -Map/--print-map
Sep 12 2020, 4:13 PM
sbc100 closed D77187: [lld][WebAssembly] Add initial support for -Map/--print-map.
Sep 12 2020, 4:13 PM · Restricted Project
sbc100 added inline comments to D77187: [lld][WebAssembly] Add initial support for -Map/--print-map.
Sep 12 2020, 4:12 PM · Restricted Project
sbc100 updated the diff for D77187: [lld][WebAssembly] Add initial support for -Map/--print-map.

feedback

Sep 12 2020, 4:11 PM · Restricted Project
sbc100 committed rG04febd30a8da: [lld][WebAssembly] Error on import/export of mutable global without `mutable… (authored by sbc100).
[lld][WebAssembly] Error on import/export of mutable global without `mutable…
Sep 12 2020, 3:12 PM
sbc100 closed D87537: [lld][WebAssembly] Error on import/export of mutable global without `mutable-globals` feature.
Sep 12 2020, 3:12 PM · Restricted Project, Restricted Project
sbc100 updated the diff for D87537: [lld][WebAssembly] Error on import/export of mutable global without `mutable-globals` feature.

Feedback

Sep 12 2020, 4:35 AM · Restricted Project, Restricted Project

Sep 11 2020

sbc100 committed rGe3e3d6eecfa5: [lld][WebAssembly] Convert a objyaml-using test to assembly (authored by sbc100).
[lld][WebAssembly] Convert a objyaml-using test to assembly
Sep 11 2020, 2:51 PM
sbc100 closed D87536: [lld][WebAssembly] Convert a objyaml-using test to assembly.
Sep 11 2020, 2:50 PM · Restricted Project