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

Alexander Shaposhnikov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 17 12:48:46 PST 2019


alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.h:32
+
+  size_t finalize();
+};
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70930





More information about the llvm-commits mailing list