This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Separate out InputGlobal from InputChunk
AbandonedPublic

Authored by ncw on Feb 16 2018, 8:04 AM.

Details

Reviewers
sbc100
ruiu
Summary

This is a follow-up to D43264, or maybe Sam could take this apply and merge into D43264?

This is a follow-on from the discussion in that issue.

I've tried to separate out InputGlobal from InputChunk, so that it doesn't derive from it (since InputChunk has relocations, but InputGlobal doesn't). This change ripples out and impacts the rest of the code a bit.

Rejected alternative:

InputGlobal (not derived from anything, this duplicates/complicates too much code)
InputChunk
  -> InputFunction
  -> InputSegment

What I've done here:

InputChunk
  -> InputSection - new class, all the relocation stuff and section offsets
                    moved from InputChunk to here
    -> InputFunction
    -> InputSegment
  -> InputGlobal

As part of that, I had a re-jigg of the handling of the function/global signature on the FunctionSymbol/GlobalSymbol objects, and discovered a bug! The current handling of undefined function signatures is wrong, oops, as evidenced by the fact that one of tests provides __cxa_at_exit with the wrong signature, yet the test passes. I discovered this when my refactoring broke the test, and then I realised I'd accidentally fixed a bug.

Other small fixes, while I was at it (should surely be rolled into D43264):

  • Fixed MarkLive to log the InputGlobals that are discarded
  • Fixed Symbol::hasOutputIndex for globals
  • Added missing DefinedGlobal::classof! Yikes.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ncw created this revision.Feb 16 2018, 8:04 AM
ruiu added a comment.Feb 16 2018, 11:01 AM

I haven't looked at the details of this patch yet, but this class hierarchy doesn't honestly look good to me, because I think global variables are not really Chunks. Chunk basically represents a piece of bytes that is copied from input file to output file. I don't think that wasm global variables fall in that category. Maybe you can still find some common parts between global variables and chunks, but it doesn't necessarily mean that we should put them in the same class hierarchy. Keeping different things different is as important as finding commonality and factor it out. I believe in this case, we should keep them separated.

InputChunk
  -> InputSection - new class, all the relocation stuff and section offsets
                    moved from InputChunk to here
    -> InputFunction
    -> InputSegment
  -> InputGlobal

Thanks Nicolas!

I split out the bug fix into a separate CL and added an addition test to exercise the signature checking:
https://reviews.llvm.org/D43399

ncw added a comment.EditedFeb 16 2018, 1:15 PM

I haven't looked at the details of this patch yet, but this class hierarchy doesn't honestly look good to me, because I think global variables are not really Chunks. Chunk basically represents a piece of bytes that is copied from input file to output file. I don't think that wasm global variables fall in that category. Maybe you can still find some common parts between global variables and chunks, but it doesn't necessarily mean that we should put them in the same class hierarchy. Keeping different things different is as important as finding commonality and factor it out. I believe in this case, we should keep them separated.

InputGlobal is a "thing" that's copied from an input file into the output file; but clearly I hear what you're saying that it doesn't fit into your idea of what a "chunk" is.

Goal: Can we come up with a suitable name or description for the behaviour that's shared between InputGlobal & InputChunk? If it's named right, would you be happy?

Background: I did do the work this morning (on your suggestion) to implement InputGlobal as a completely separate class, and I got the code all working, but it was nasty in several places. There was extra code duplication. That's how I ended up with this patch, after going down that route first.

The fact is, InputGlobal shares a number of members with InputChunk - namely the Live flag, the Comdat handling, the output-index handling.

I know sometimes that shared members can be copy-pasted into another class... but what really counts for determining whether to make a base class is shared behaviour. In this case we do have that, with several places where we operate on an InputThing in a generic way:

  • MarkLive sets the live bit on InputChunks/InputGlobals, so it's awkward if they don't share a base class. You end up doubling the amount of code in MarkLive's critical loop, if it requires a switch statement to set the Live bit depending on whether it's an InputGlobal or InputChunk that's being GC'd.
  • Similarly when writing out Wasm exports, we want to iterate over all the InputThings together, and make use of the members that InputGlobal/InputChunk have in common
  • Some more instances in Writer that operate on the "chunk" for a defined symbol - would need to copy-paste that code if some symbols have an InputGlobal and some symbols have an InputChunk, and there's nothing common between them
  • And also for Comdat handling we'll need to handle them together...

Let's just imagine that we have this hierarchy - could you suggest some names that would work for you, to describe these classes:

CLASS_ONE // represents a "thing" or "item" to be copied from an input file to an output file. 
          // Interface: getName(), getFileName(), Live, getComdat()
          // Suggested names: InputObject, InputChunk, Gcable.... do any of those resonate?
  -> CLASS_TWO // represents a block of binary data to be copied from an input file to an output file
               // Interface: getSectionOffset(), getRelocations()
               // Suggested names: InputSection, InputBytes, InputChunk...
    -> InputSegment (data section)
    -> InputFunction (code section)
  -> InputGlobal // represents a block of defined data for a special type of data symbol
ruiu added a comment.Feb 16 2018, 1:54 PM

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?

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?

I'm working on a simpler abstraction now.

ncw added a comment.EditedFeb 17 2018, 3:16 PM

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.

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.

I think the important distinction is "OutputSection" vs "SyntheticOutputSection". The former is constructed in parallel based on memcpy+relocations. The latter is created from scratch by the linker. You are suggesting that we might want to make the global sections into non-synthetic section. We might want to do that one day, but I don't think we should for this initial patch. I think we are already getting a bit ahead of ourselves by even including first class globals in this patch (I was hoping to switch to symbol table without introducing a new symbol type but I don't really think that can be avoided).

ncw abandoned this revision.Feb 23 2018, 7:37 AM

Abandoning, now that symbol table has landed.

Well done @sbc100 (and thanks) on getting it through!