[PATCH] D70930: [llvm-objcopy][WebAssembly] Initial support for wasm in llvm-objcopy

[Derek Schuff via Phabricator via llvm-commits]

dschuff marked an inline comment as done.
dschuff added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.h:32
+
+  size_t finalize();
+};
----------------
alexshap wrote:
> alexshap wrote:
> > dschuff wrote:
> > > alexshap wrote:
> > > > sbc100 wrote:
> > > > > alexshap wrote:
> > > > > > if I'm not mistaken this method is not really "finalize", 
> > > > > > the main thing what it does - it properly encodes Sections + computes the total size.
> > > > > > 
> > > > > > What would you say to the following reorganization of this code:
> > > > > > 
> > > > > > Writer.h:
> > > > > > 
> > > > > >    classWriter {
> > > > > >         ....
> > > > > >         Error write(); 
> > > > > > 
> > > > > >     private:   
> > > > > >         uint64_t totalSize() const { ... }
> > > > > >     };
> > > > > > 
> > > > > > Writer.cpp:
> > > > > >     uint64_t calculateSerializedSectionSize(const Section &S) {
> > > > > >          uint64_t Size = S.Content.size();
> > > > > >          if (S.SectionType == WASM_SEC_CUSTOM)
> > > > > >               Size += (getULEB128Size(S.Name.size()) + S.Name.size());
> > > > > >          return Size;
> > > > > >      }
> > > > > > 
> > > > > >      void writeSection(const Section &S, uint8_t* Out) {
> > > > > >           ....
> > > > > >      }
> > > > > > 
> > > > > >      uint64_t Writer::totalSize() const {
> > > > > >          ....
> > > > > >      }
> > > > > > 
> > > > > >      Error Writer::write() {
> > > > > >          ....
> > > > > >      }
> > > > > > 
> > > > > > Side note: it looks like in this case we eliminate SectionData - decrease the number of copies/ memory consumption.
> > > > > In the lld codebase we use "finalize" on section to mean exactly that: "properly encodes Sections + computes the total size".
> > > > > 
> > > > > Do you have other other meaning of the work finalize in mind?   Perhaps `finalizeContents()` would be more explicit?
> > > > it feels like it's unnecessary to keep around one extra copy (SectionData)  (especially if it doesn't contain any fields which need to be computed from the final layout (i.e. global offsets, indices, etc)) + that way it looks simpler to me unless I'm missing smth.
> > > Actually the root problem here is that `SectionData` is totally misnamed. It's not actually the section payload at all, it's just the header, which does need to be computed from the final layout. (that's why it's a SmallVector). I think I originally had both headers and data and I must have renamed/eliminated the wrong name when I change it.
> > > I just renamed it `SectionHeader` and it all hopefully makes more sense now.
> > > 
> > > In light of that, `finalize` does more than just compute the size, it generates the header as well, and as @sbc100 says, matches the way we use the term in the wasm backend and lld.
> > I see, thank you for the additional context, the new name makes more sense,
> > but I'm still kinda curious - what are the advantages of keeping this around vs "writing on the fly" ?
> > the latter looks simpler + a little bit less code 
> + regardless of the matter, i think the code which generates a SectionHeader for a given Section should be put into a helper function + it would be good to add a brief comment explaining the format + a link to the detailed specification (for those who will read this code in the future)
I think the reason it's split into a separate step is that because of the LEB128 encodings (ubiquitous in wasm), the size of the section isn't known until the encodings are done. Here it only shows up in header generation where the size includes the encoded length of the section name, but there are other cases (especially when dealing with relocations and instructions) where there are more LEBs that we might prefer to either leave encoded with extra bytes (for rewriting later) or compress to reduce binary size.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70930/new/

https://reviews.llvm.org/D70930