sbc100 (Sam Clegg)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 16 2016, 10:22 AM (74 w, 6 d)

Recent Activity

Yesterday

sbc100 updated the summary of D41366: [WebAssembly] Remove DataSize from linking metadata section.
Thu, Feb 22, 9:59 PM
sbc100 added reviewers for D41366: [WebAssembly] Remove DataSize from linking metadata section: dschuff, ncw, jgravelle-google.
Thu, Feb 22, 9:56 PM
sbc100 accepted D43524: Simplify..

A better change title is probably needed :)

Thu, Feb 22, 9:49 PM
sbc100 added a comment to D43523: Remove SymbolTable::addUndefined and add SymbolTable::addUndefined{Function,Global}.

Sorry I didn't see this earlier. Looks good but will need rebase since the symtab change langed.

Thu, Feb 22, 9:49 PM
sbc100 accepted D43670: Add more items to lld 6.0 release note.

Great!

Thu, Feb 22, 9:45 PM
sbc100 updated the diff for D41366: [WebAssembly] Remove DataSize from linking metadata section.
  • rebase
Thu, Feb 22, 9:45 PM
sbc100 added a comment to D43264: [WebAssembly] Add explicit symbol table.

I fixed the DefinedData constructor args. The rest we can follow up with. Thanks for all the reviewing!

Thu, Feb 22, 9:12 PM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • remove default args
Thu, Feb 22, 9:08 PM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • typos
  • feedback
  • Simplify address calculations for data symbols
Thu, Feb 22, 3:38 PM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • feedback
Thu, Feb 22, 2:13 PM
sbc100 added a comment to D43264: [WebAssembly] Add explicit symbol table.

wabt change is ready to go: https://github.com/WebAssembly/wabt/pull/769

Thu, Feb 22, 2:13 PM
sbc100 added inline comments to D43264: [WebAssembly] Add explicit symbol table.
Thu, Feb 22, 2:13 PM

Wed, Feb 21

sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • fix typos in comment
Wed, Feb 21, 10:46 AM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • rebase
Wed, Feb 21, 10:43 AM
sbc100 retitled D43587: [WebAssembly] Use make<> rather then make_unique<>. NFC. from [WebAssembly] Use make<> rather then make_unique<> to [WebAssembly] Use make<> rather then make_unique<>. NFC..
Wed, Feb 21, 10:31 AM
sbc100 added a reviewer for D43588: [WebAssembly] Rename member DefinedFunctions -> InputFunctions. NFC.: ruiu.
Wed, Feb 21, 10:31 AM
sbc100 retitled D43588: [WebAssembly] Rename member DefinedFunctions -> InputFunctions. NFC. from [WebAssembly] Rename member DefinedFunctions -> InputFunctions to [WebAssembly] Rename member DefinedFunctions -> InputFunctions. NFC..
Wed, Feb 21, 10:31 AM
sbc100 created D43588: [WebAssembly] Rename member DefinedFunctions -> InputFunctions. NFC..
Wed, Feb 21, 10:28 AM
sbc100 added a comment to D43587: [WebAssembly] Use make<> rather then make_unique<>. NFC..

Is my rational correct here? Based on your feedback from my other change it seems like this should be preferred?

Wed, Feb 21, 10:24 AM
sbc100 retitled D43587: [WebAssembly] Use make<> rather then make_unique<>. NFC. from [WebAssebmly] Use make<> rather then make_unique<> to [WebAssembly] Use make<> rather then make_unique<>.
Wed, Feb 21, 10:24 AM
sbc100 created D43587: [WebAssembly] Use make<> rather then make_unique<>. NFC..
Wed, Feb 21, 10:24 AM
sbc100 added a reviewer for D43587: [WebAssembly] Use make<> rather then make_unique<>. NFC.: ruiu.
Wed, Feb 21, 10:24 AM
sbc100 added inline comments to D43264: [WebAssembly] Add explicit symbol table.
Wed, Feb 21, 10:17 AM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • feedback
  • clang-format
  • feedback
Wed, Feb 21, 10:17 AM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • add Symbol::isLive()
Wed, Feb 21, 9:43 AM
sbc100 added inline comments to D43264: [WebAssembly] Add explicit symbol table.
Wed, Feb 21, 9:27 AM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • feedback
Wed, Feb 21, 9:27 AM

Tue, Feb 20

sbc100 added a comment to D43540: [WebAssembly] Enable -Werror=strict-prototypes by default.

Hmm.. actually this is probably going to break the waterfall pretty badly. But maybe we can fix it by re-disabling this explicitly when we build the gcc tests.

Tue, Feb 20, 5:43 PM
sbc100 accepted D43540: [WebAssembly] Enable -Werror=strict-prototypes by default.

Might want to line wrap your change description.

Tue, Feb 20, 5:43 PM
sbc100 abandoned D43535: - rebase.
Tue, Feb 20, 4:45 PM
sbc100 created D43535: - rebase.
Tue, Feb 20, 4:31 PM
sbc100 created D43534: [WebAssembly] Move lambda declaration output of loop.
Tue, Feb 20, 4:27 PM
sbc100 added a reviewer for D43534: [WebAssembly] Move lambda declaration output of loop: ruiu.
Tue, Feb 20, 4:27 PM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • revert parts
Tue, Feb 20, 4:17 PM
sbc100 updated the diff for D43264: [WebAssembly] Add explicit symbol table.
  • Don't use InputChunk to model globals
Tue, Feb 20, 4:04 PM
sbc100 accepted D43526: Inline printHelp..
Tue, Feb 20, 2:23 PM
sbc100 accepted D43527: Handle --version before handling --mllvm..
Tue, Feb 20, 2:23 PM
sbc100 accepted D43525: Inline a trivial ctor..
Tue, Feb 20, 2:19 PM
sbc100 updated subscribers of D43525: Inline a trivial ctor..

Could you please always prefix the differential's subject with the appropriate tag,
e.g. [lld][WASM], and do specify the appropriate repo and tags.

Otherwise people actually have to look at the diff to understand
what it is about.

Tue, Feb 20, 2:16 PM
sbc100 updated the summary of D43529: Consistent use of header file for ICF and MarkLive.
Tue, Feb 20, 1:59 PM
sbc100 updated the diff for D43529: Consistent use of header file for ICF and MarkLive.
  • format
Tue, Feb 20, 1:59 PM
sbc100 updated the diff for D43529: Consistent use of header file for ICF and MarkLive.
  • format
Tue, Feb 20, 1:56 PM
sbc100 created D43529: Consistent use of header file for ICF and MarkLive.
Tue, Feb 20, 1:55 PM
sbc100 added a reviewer for D43528: Consistent (non) use of empty lines in include blocks: ruiu.
Tue, Feb 20, 1:41 PM
sbc100 updated the diff for D43528: Consistent (non) use of empty lines in include blocks.
  • revert
Tue, Feb 20, 1:41 PM
sbc100 created D43528: Consistent (non) use of empty lines in include blocks.
Tue, Feb 20, 1:41 PM
sbc100 accepted D43517: Use more early returns in SymbolTable.cpp..

Nice!

Tue, Feb 20, 1:03 PM
sbc100 added a comment to D43496: [WebAssembly] Split addDefined into two different methods. NFC..

Done.

Tue, Feb 20, 10:53 AM
sbc100 updated the diff for D43496: [WebAssembly] Split addDefined into two different methods. NFC..
  • feedback
Tue, Feb 20, 10:53 AM
sbc100 added a comment to D43517: Use more early returns in SymbolTable.cpp..

This is going to conflict pretty badly with https://reviews.llvm.org/D43496... which are we going to land first? :)

Tue, Feb 20, 10:48 AM
sbc100 added a reviewer for D43516: [WebAssembly] Remove unused header: ruiu.
Tue, Feb 20, 10:21 AM
sbc100 created D43516: [WebAssembly] Remove unused header.
Tue, Feb 20, 10:21 AM
sbc100 added inline comments to D43496: [WebAssembly] Split addDefined into two different methods. NFC..
Tue, Feb 20, 9:55 AM
sbc100 updated the diff for D43493: [WebAssembly] Remove InputChunk from Symbol baseclass. NFC..
  • feedback
  • clang-format
Tue, Feb 20, 9:48 AM
sbc100 updated the diff for D43493: [WebAssembly] Remove InputChunk from Symbol baseclass. NFC..
  • feedback
Tue, Feb 20, 9:39 AM
sbc100 added inline comments to D43493: [WebAssembly] Remove InputChunk from Symbol baseclass. NFC..
Tue, Feb 20, 9:39 AM

Mon, Feb 19

sbc100 added a reviewer for D43496: [WebAssembly] Split addDefined into two different methods. NFC.: ruiu.
Mon, Feb 19, 5:24 PM
sbc100 updated the diff for D43496: [WebAssembly] Split addDefined into two different methods. NFC..
  • make static
Mon, Feb 19, 5:24 PM
sbc100 created D43496: [WebAssembly] Split addDefined into two different methods. NFC..
Mon, Feb 19, 5:22 PM
sbc100 added a reviewer for D43493: [WebAssembly] Remove InputChunk from Symbol baseclass. NFC.: ruiu.
Mon, Feb 19, 4:45 PM
sbc100 created D43493: [WebAssembly] Remove InputChunk from Symbol baseclass. NFC..
Mon, Feb 19, 4:42 PM
sbc100 retitled D43493: [WebAssembly] Remove InputChunk from Symbol baseclass. NFC. from [WebAssembly] Remove InputChunk from Symbol baseclass to [WebAssembly] Remove InputChunk from Symbol baseclass. NFC..
Mon, Feb 19, 4:42 PM
sbc100 added a reviewer for D43492: [WebAssembly] Check signatures of weakly defined funtions too: ruiu.
Mon, Feb 19, 4:40 PM
sbc100 created D43492: [WebAssembly] Check signatures of weakly defined funtions too.
Mon, Feb 19, 4:40 PM
sbc100 added inline comments to D43491: [WebAssembly] Do not create a temporary data structure for relocations..
Mon, Feb 19, 4:38 PM
sbc100 added a comment to D43491: [WebAssembly] Do not create a temporary data structure for relocations..

This is much better. Thank you. I particularly like the elimination of that random struct.

Mon, Feb 19, 4:35 PM
sbc100 accepted D43484: Inline applyRelocations into writeTo..

This doesn't seem strictly better to me. Seems like it only increases the complexity and the indentations of the code, and while decreasing re-usability. Is there a specific benefit? Shouldn't we prefer shorter and less nested functions?

Mon, Feb 19, 4:31 PM
sbc100 retitled D43476: [WebAssembly] Rename symbols types in preparation to adding wasm globals. NFC. from [WebAssembly] Rename symbols types in preparation to adding wasm globals to [WebAssembly] Rename symbols types in preparation to adding wasm globals. NFC..
Mon, Feb 19, 11:48 AM
sbc100 added reviewers for D43476: [WebAssembly] Rename symbols types in preparation to adding wasm globals. NFC.: ruiu, ncw.
Mon, Feb 19, 11:48 AM
sbc100 created D43476: [WebAssembly] Rename symbols types in preparation to adding wasm globals. NFC..
Mon, Feb 19, 11:47 AM
sbc100 accepted D43435: Expand a lambda that is used only once..

I was considering doing that myself, and managed to persuade myself it was more readable with the lamda since it makes the loop itself self-documenting and really easy to read. But I can see that single use lambda are probably not a good pattern and that a commit is probably just as good. BTW, do you now if there is any runtime different between these two (given a decent compiler)?

Mon, Feb 19, 10:48 AM
sbc100 added a comment to D43434: Define toString(wasm::InputChunk *) and use that in MarkLive.cpp..

Can you put [WebAssembly] at the start of the change title?

Mon, Feb 19, 10:43 AM
sbc100 accepted D43434: Define toString(wasm::InputChunk *) and use that in MarkLive.cpp..
Mon, Feb 19, 10:43 AM
sbc100 added a comment to D43391: [WebAssembly] Separate out InputGlobal from InputChunk.
In D43391#1011480, @ncw wrote:

I had a great idea on this last night - I think I've been misrepresenting globals somewhat. This came to me after Andreas Rossberg answered a question I had about globals.

They're actually more like functions than data! Our rather, they are used/accessed as data at runtime, but the Globals section of the Wasm file actually contains a function body for each global, which contains executable code that's run through the interpreter to give the global its initial value.

I kept saying "globals are like a chunk in every way except that they don't have relocations" - but that's wrong, they should have relocations! The "body" for a global can contain a get_global instruction, which requires a relocation for its operand, so what's really been misleading is that thelinking conventions for wasm simply forgot to mention that the linker needs to handle a "reloc.GLOBAL" section.

I know what you're thinking - "Nick, the clang front-end currently doesn't emit globals that contain a get_global instruction, so we don't need to process relocations for them".

But the linking conventions should still specify it, regardless of the fact that the clang front-end doesn't yet emit them. One thing's sure, fPIC and shared-objects and threading will find more uses for globals than we had before.

More to the point - the "wasmy" way to think of globals is as a runtime data register, which is packaged in the wasm file with a function body used to initialize it. That could be reflected in LLD's model. (A typical function body for a global is something like a single "ret i32 <immediate>" instruction, or "const.i32 NNN" in wasm assembly, but a few other forms are legal.)

To conclude: I'm not trying to complicate things for globals, I'm just trying to use exactly the same tried-and-tested abstraction for them, that LLD is already right now using for functions and segments, rather than try and make up something new.

And since globals can contain relocations after all, they look a lot more like functions; treating them as chunks like the rest really should give us basically the least amount of code overall, as well as ensuring the various symbol types work in the same way for "free".

Edit - I'll see if I have any time on Monday to confirm my conjecture on the potential simplification of the code, by updating this PR to model globals as a chunk like functions. I had been intending that later on as a tidy-up, before realising that globals would actually benefit from relocation processing.

Mon, Feb 19, 10:38 AM

Fri, Feb 16

sbc100 updated the summary of D43422: [WebAssembly] Remove unneeded classifer methods from Symbol class. NFC..
Fri, Feb 16, 4:39 PM
sbc100 created D43422: [WebAssembly] Remove unneeded classifer methods from Symbol class. NFC..
Fri, Feb 16, 4:37 PM
sbc100 added a comment to D43420: Use toString to print out garbage-collected sections..

Why not change wasm at the same time?

Fri, Feb 16, 4:17 PM
sbc100 added inline comments to D43416: [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC..
Fri, Feb 16, 3:50 PM
sbc100 added a comment to D43405: [WebAssembly] Remove unneeded Chunk::getFileName() method. NFC..

LGTM

But I believe a better way of doing this kind of stuff is to define toString(InputChunk *). toString() function is a common interface to stringize objects to create debug messages.

Fri, Feb 16, 3:49 PM
sbc100 updated the diff for D43416: [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC..
  • rename
  • remove_default
Fri, Feb 16, 3:42 PM
sbc100 added a reviewer for D43405: [WebAssembly] Remove unneeded Chunk::getFileName() method. NFC.: ruiu.
Fri, Feb 16, 3:39 PM
sbc100 added reviewers for D43416: [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC.: ruiu, ncw.
Fri, Feb 16, 3:39 PM
sbc100 retitled D43416: [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC. from [WebAssembly] Simply FunctionSymbol get/set/hasFunctionType. NFC. to [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC..
Fri, Feb 16, 3:39 PM
sbc100 abandoned D43417: [WebAssembly] Add wasm-ld to lit tool_patterns.

Ha!

Fri, Feb 16, 3:34 PM
sbc100 added a reviewer for D43417: [WebAssembly] Add wasm-ld to lit tool_patterns: ruiu.
Fri, Feb 16, 3:20 PM
sbc100 created D43417: [WebAssembly] Add wasm-ld to lit tool_patterns.
Fri, Feb 16, 3:20 PM
sbc100 retitled D43416: [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC. from [WebAssembly] Simpliy FunctionSymbol get/set/hasFunctionType to [WebAssembly] Simply FunctionSymbol get/set/hasFunctionType. NFC..
Fri, Feb 16, 3:02 PM
sbc100 updated the diff for D43416: [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC..
  • rename
Fri, Feb 16, 3:01 PM
sbc100 created D43416: [WebAssembly] Simplify FunctionSymbol get/set/hasFunctionType. NFC..
Fri, Feb 16, 3:01 PM
sbc100 accepted D43408: Do not print out "no input files" twice..

Great!

Fri, Feb 16, 2:54 PM
sbc100 added a comment to D43408: Do not print out "no input files" twice..

I noticed we don't have a test for this case.. would you mind adding one ?

Fri, Feb 16, 2:46 PM
sbc100 added a comment to D43264: [WebAssembly] Add explicit symbol table.

It also didn't feel right to have InputGlobal to inherit from InputChunk. For me the Chunks are really for the non-synthetic sections. That is the ones construct using memory copied from the input files + relocations.

Fri, Feb 16, 2:43 PM
sbc100 accepted D43407: Use wasm-ld instead of "lld -flavor wasm"..
Fri, Feb 16, 2:39 PM
sbc100 added a comment to D43391: [WebAssembly] Separate out InputGlobal from InputChunk.

I can say that handling global variables as Chunks as you did in this patch isn't an intended use of Chunk class, but in order to answer to your question as to what is a better way of abstracting it, I need to experiment various ideas until I find something that fits snugly to the entire design of lld.

The fact that global variables is a "thing" that copied from input files to an output file doesn't mean that that need to be abstracted as Chunks. Everything in the linker, except the one given via the command line, are after all created from input files, and most of them are in some way copied to the output file. Symbols are created for input files and copied to the output symbol table, for example. As I wrote, Chunk essentially represents a contiguous bytes in input files, and I'm not convinced that global variables have that property.

I'd think you are perhaps overthinking about the design. In lld, we are careful not to be too clever. We are trying to keep the class hierarchy simple and shallow, and we are trying to not abstract things too much. And I believe you can find that design principle throughout the lld code.

When we find that something needs to be abstracted in order to make program better, I'm totally fine with doing that. But I don't want to design something too much beforehand, because in many cases that's inappropriate or wouldn't be needed in the future. I'm not worried that the fundamental design of wasm lld is wrong (it's based on the proven design!), so even if we have to refactor code, that's not a big task. So, can we just keep global variables as a symbol until we find that that's not a suitable representation?

Fri, Feb 16, 2:39 PM
sbc100 added inline comments to D43406: Merge two small functions and add comments..
Fri, Feb 16, 2:36 PM
sbc100 added inline comments to D43406: Merge two small functions and add comments..
Fri, Feb 16, 1:01 PM
sbc100 added a comment to D43406: Merge two small functions and add comments..

By the way, what is InputChunk::copyRelocations for? That function copies relocations from other InputChunk, but it's not clear to me why we need to do that.

Fri, Feb 16, 1:00 PM
sbc100 accepted D43406: Merge two small functions and add comments..
Fri, Feb 16, 12:53 PM
sbc100 created D43405: [WebAssembly] Remove unneeded Chunk::getFileName() method. NFC..
Fri, Feb 16, 12:43 PM
sbc100 accepted D43403: Refactor wasm/WriterUtil.{cpp,h}..

Thanks!

Fri, Feb 16, 12:35 PM