[PATCH] D43391: [WebAssembly] Separate out InputGlobal from InputChunk

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added a comment.

In https://reviews.llvm.org/D43391#1010450, @ruiu wrote:

> 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 you 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, if it can't set the Live bit without having a switch statement to select between InputGlobal/InputChunk.
- 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 of 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


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43391