This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][WebAssembly] Initial support for wasm in llvm-objcopy
ClosedPublic

Authored by dschuff on Dec 2 2019, 3:54 PM.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Dec 3 2019, 1:45 AM
llvm/test/tools/llvm-objcopy/Wasm/basic-archive-copy.test
12

Nit: too many blank lines.

16

'##'

90

Use '#' characters here.

llvm/test/tools/llvm-objcopy/Wasm/basic-copy.test
2

Same as above.

8–9

What's stopping llvm-objcopy using the 5-byte LEBs?

FWIW, in ELF, we don't require a basic copy to produce identical output to the input, because the input might be written in an order style (e.g. the section header table might appear in a different place to "normal", there might be gaps in the object which objcopy compresses etc). Assuming this applies for WASM, you shouldn't try to do a binary comparison, but rather test the key things are all identical.

llvm/tools/llvm-objcopy/Wasm/Object.cpp
50

Is there any structure that you can do a sizeof operation on or similar to avoid a literal here (and the need of the comment)?

76

Put a blank line before each grouping here. Also, comments need leading capital letters and trailing full stops.

llvm/tools/llvm-objcopy/Wasm/Object.h
61

You're already in the llvm namespace. Can't you just do wasm::WasmObjectHeader?

63

This line of the comment isn't useful. The names of the functions show this. Please delete.

llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp
40–42

Should this be an error? In ELF, an empty section is legitimate, and there's no reason it couldn't generate an empty file in this case.

54

This may want to be errc::invalid_argument, since the process hasn't failed to parse the file.

115

Copy-paste error?

dschuff updated this revision to Diff 231932.Dec 3 2019, 9:36 AM
dschuff marked 17 inline comments as done.
  • Add test that removed section is gone
  • Address round 1 of review comments

Thanks for the detailed review. I just uploaded a diff to address the comments (other than splitting the patch apart).

llvm/test/tools/llvm-objcopy/Wasm/add-section.test
2

This started out as a copy of test/tools/llvm-objcopy/ELF/add-section.test which has a similar structure. Since you've asked to start without this functionality anyway, I can back this test out entirely for now.

llvm/test/tools/llvm-objcopy/Wasm/basic-copy.test
8–9

Nothing is stopping it; I just decided to match clang's behavior instead of yaml2obj's (as the comment mentions, they are different). I'll just clarify the comment, since as you say, there's not really any reason to change it.

llvm/tools/llvm-objcopy/Wasm/Object.h
61

No:
/s/llvm-upstream/llvm-project/llvm/tools/llvm-objcopy/Wasm/Object.h:60:3: error: no type named 'WasmObjectHeader' in namespace 'llvm::objcopy::wasm'; did you mean '::llvm::wasm::WasmObjectHeader'?

llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp
40–42

Actually you're right; I believe custom sections can be empty (Wasm has a distinction between "known" sections whose structure is specified by the standard and "custom" sections which are not interpreted by the VM). Since we only try to support custom sections for now, I'll remove this check.

dschuff updated this revision to Diff 231934.Dec 3 2019, 9:48 AM
  • Remove add/remove/dump section support

Great to see this happening.

I wonder if the directory name should be called just wasm.. like e.g. lld/test/wasm and lld/wasm, We do have a mixture of wasm, 'Wasm` and WebAssembly. I've been thinking of WebAssembly as the target and wasm as the object/container format. So in this case I think just wasm makes the most sense for the directory name.

Do you mean wasm instead of Wasm?

dschuff updated this revision to Diff 231943.Dec 3 2019, 10:21 AM
  • rename Wasm directories to wasm
sbc100 added inline comments.Dec 3 2019, 10:24 AM
llvm/test/tools/llvm-objcopy/Wasm/basic-archive-copy.test
2

I looks like objcopy might be the exception here but normally when writing tests in a given format such as this we give them the file extension of the format. So this file should be a .yaml maybe? One of the advantages of this is that you get syntax highlighting by default.

Maybe we should land a rename of the existing tests to follow this convention first?

llvm/tools/llvm-objcopy/Wasm/Object.cpp
58

Seems like kind of a shame we have so many different places where we write the object format. This existing ones that I know of are:

  1. llvm/lib/MC/WasmObjectWriter.cpp
  2. llvm/lib/ObjectYAML/WasmEmitter.cpp
  3. lld/wasm/Writer.cpp

So this is added a 4th. But maybe its the same for ELF. I wonder if it would make sense to add a writing capability to lib/Object to unify these. I guess they are all subtly different so maybe it doesn't make sense?

llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp
54

Is this assert useful? The contract with the Expected is that that the object pointer is valid if its not an error right? Also "deserialize" is a strange word here.. read or load would make more sense to me).

dschuff marked 2 inline comments as done.Dec 3 2019, 10:53 AM
dschuff added inline comments.
llvm/tools/llvm-objcopy/Wasm/Object.cpp
58

It is a bit odd that there is a completely independent data model of object files here (the ELF one is particularly complex but I don't have a particular intuition about how much duplication there is for ELF). For wasm (assuming we want to model symbols and relocations against custom sections), my hope was that we could continue to treat the known sections and most custom sections as opaque blobs and hopefully reuse the MC logic for symbols, which would mean that this code wouldn't get too much more complex. Not sure about how much work/duplication symbols would be though.

llvm/tools/llvm-objcopy/Wasm/WasmObjcopy.cpp
54

I think if Expected holds a pointer/smart pointer, it can still be null?

I looks like objcopy might be the exception here but normally when writing tests in a given format such as this we give them the file extension of the format. So this file should be a .yaml maybe? One of the advantages of this is that you get syntax highlighting by default.

Maybe we should land a rename of the existing tests to follow this convention first?

objcopy isn't the exception. It's common practice for YAML tests throughout the binutils to have .test as an extension, because .yaml isn't in the default set of extensions that lit recognises as a test. I don't object to a mass renaming, but it should probably be done for each of the tools and probably needs to include a change in the default suffixes too (which might cause a problem in some places where a separate YAML file is used as an input...).

llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test
1 ↗(On Diff #231943)

Maybe rephrase this slightly:

"Test a basic copy of an archive containing a WASM object."

15–16 ↗(On Diff #231943)

This is probably not needed: you already have basic-copy.test, which shows that objects are copied correctly, so the comparison of the extracted object is probably sufficient.

42 ↗(On Diff #231943)

As mentioned in the now-deleted test, please remove the excessive whitespace, so that the Values in any given block line up neatly, but as close as possible to their tags, i.e.

--- !WASM
FileHeader:
  Version: 0x00000001
Sections:
  - Type: TYPE
    Signatures:
      - Index: 0
        ParamTypes:
          - I32
        ReturnTypes:
          - F32
      - Index: 1
        ParamTypes:
          - I32
          - I64
        ReturnTypes: []
# etc
      - Type:   R_WASM_TABLE_INDEX_SLEB
        Index:  0
        Offset: 0x00000006
73 ↗(On Diff #231943)

Not knowing anything really about WASM, how much of the stuff in this output is really necessary for the test? Can, e.g. the Body value here be reduced down to a minimal content?

93–94 ↗(On Diff #231943)

You don't need these two lines.

llvm/test/tools/llvm-objcopy/wasm/basic-copy.test
4 ↗(On Diff #231943)

Prefer -o to shell redirection.

10 ↗(On Diff #231943)

Same comments apply to this YAML as in the other test.

llvm/tools/llvm-objcopy/Wasm/Object.cpp
58

I can't comment about WASM, but to some degree, ELF llvm-objcopy needs to have a different internal representation of an Object to that of the library, because the existing Object library object file there doesn't really support mutation. That's largely why things are written the way they were. @jakehehrlich might have more comments on that though.

llvm/tools/llvm-objcopy/wasm/Object.h
65 ↗(On Diff #231943)

Can we get rid of this for this version (i.e. add it later when they are needed)?

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
38 ↗(On Diff #231943)

clang-format?

41 ↗(On Diff #231943)

I'd probably just change this comment so that it is future proof "unsupported flag specified", or something like that. Also, we usually use lower-case first letter for error/warning messages.

dschuff updated this revision to Diff 232200.Dec 4 2019, 1:55 PM
dschuff marked 12 inline comments as done.
  • Review 2
llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test
42 ↗(On Diff #231943)

OK, but is it really worthwhile to deviate from the default output format of obj2yaml and hand-edit everything just so there's less whitespace? It doesn't look any more readable to me.

73 ↗(On Diff #231943)

I guess for the purposes of this test it doesn't actually matter whether the function body is valid wasm code or not. I do want to have some known (CODE, TYPE, etc) and linking/reloc sections in the test so that we can ensure that both code that is and is not understood by the object file layer doesn't get modified. That also means that the body has to be long enough that 5-byte relocations against it make sense.

93–94 ↗(On Diff #231943)

Removing the readobj test above means removing all of this anyway.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
41 ↗(On Diff #231943)

That's more future-proof for me, but much less useful for a user. In order to figure out what *is* supported if someone gets this error, they'd have to keep removing flags until it works. I did update the case though.

jhenderson added inline comments.Dec 5 2019, 2:08 AM
llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test
42 ↗(On Diff #231943)

Excessive whitespace for me at least makes it harder to read.

An obj2yaml-generated document is not much better than a canned-object file in most test cases. The vast majority (all?) of our ELF tests use YAML that has been hand-written to make sure it covers the things we care about and nothing more. obj2yaml may well generate significantly more output than is needed to test the different bits that are really being implemented, so whilst it may be useful to provide a basis, the output really should be cut down.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
41 ↗(On Diff #231943)

Yeah, I'm not particularly happy with how this is implemented in COFF or Mach-O either. Note that when you do add support for a single switch, you'll need to message, and that will just get unwieldy as more options are added. Perhaps we need to figure out a better method of resolving this, probably with some sort of command-line validation layer somewhere that goes over all switches and sees if they're supported.

llvm/tools/llvm-objcopy/wasm/Object.h
1 ↗(On Diff #232200)

I'd prefer to follow the structure which we use for other formats (MachO, COFF, ELF), i.e. put the description of the model into Object.h/Object.cpp (the class Object etc), and put the classes Reader and Writer into separate files, even if they are small at the moment, they might grow in the future, it makes things a little bit easier to read.

dschuff updated this revision to Diff 232358.Dec 5 2019, 8:39 AM
dschuff marked an inline comment as done.
  • Split Reader/Writer into separate files

I believe I've addressed all the feedback now.

llvm/tools/llvm-objcopy/wasm/Object.h
1 ↗(On Diff #232200)

I don't really mind doing this, but I will note that ELF also has its Readers and Writers in Object.h

jhenderson added inline comments.Dec 6 2019, 1:31 AM
llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test
86–87 ↗(On Diff #232358)

Minor nit: add an extra space between Kind/Name and their values to match the Index 1 symbol.

llvm/tools/llvm-objcopy/wasm/Object.h
1 ↗(On Diff #232200)

I wouldn't mind somebody changing ELF to split them out too. Having them in the same file always confuses me initially.

llvm/tools/llvm-objcopy/wasm/Writer.cpp
26–28 ↗(On Diff #232358)

I think we've been trying to get rid of these sort of non-obvious auto usages within llvm-objcopy. Whilst I can tell that S and Header are likely some kind of section and section header respectively, it's not obvious to me what their type is. Please replace them with the concrete type.

31 ↗(On Diff #232358)

Does wasm define a constant for this 0 case here?

35 ↗(On Diff #232358)

What is the 5 here? Is there a constant for it somewhere if applicable?

40 ↗(On Diff #232358)

Same sort of comment as above. What are the 1 and 5?

52 ↗(On Diff #232358)

Does wasm have a concept of endianness that we need to care about here?

55 ↗(On Diff #232358)

i -> I

Also, I believe the standard LLVM style is:

for (size_t I = 0, S = SectionHeaders.size(); I < S; ++I)

57 ↗(On Diff #232358)

Don't use auto here please. It's not clear what the type is again.

dschuff updated this revision to Diff 233408.Dec 11 2019, 10:33 AM
dschuff marked 8 inline comments as done.
  • Review comments 3
llvm/tools/llvm-objcopy/wasm/Writer.cpp
35 ↗(On Diff #232358)

No constant (it's just the max encoded LEB size that can encode 32 bits) but added a comment.

52 ↗(On Diff #232358)

No, the spec says that these header values here are both little-endian and we are not trying to interpret them anywhere. All the other values in the format are either 1 byte or LEB-encoded.

Only one small pair of questions from me, though absolutely others should approve this too.

llvm/tools/llvm-objcopy/wasm/Writer.cpp
35 ↗(On Diff #232358)

Okay, thanks. Why 32-bit though? Also, do we need to handle data that is greater in size than max 32-bits in some way?

Thanks for all your input!
Who else needs to review this? Can you say explicitly, so we can keep this from becoming one of those reviews that hangs in limbo for a long time without any clear path forward?

dschuff marked an inline comment as done.Dec 12 2019, 10:04 AM
dschuff added inline comments.
llvm/tools/llvm-objcopy/wasm/Writer.cpp
35 ↗(On Diff #232358)

The spec (e.g. for the module format) specifies the size of each kind of field, and also specifies how the values are encoded. In the MVP/1.0 version of wasm everything has 32-bit types (with a few exceptions, e.g. i64/f64 constants; but definitely everything in the module format). A future extension will allow 64-bit pointers and memory sizes and will likely also extend some of the module-level encodings as a result. That would likely be treated by LLVM as a separate architecture and possibly a separate object format variant akin to ELFCLASS64.

llvm/tools/llvm-objcopy/wasm/Object.h
21 ↗(On Diff #233408)

Sorry for the delays with code review, I'll try to catch up on all the diffs in my backlog soon.
I'm not an expert in WASM format, so please take my comments with a grain of salt.

Okay, what would you say to the following approach ?

struct Section {
   uint8_t Type;
   StringRef Name;
   ArrayRef<uint8_t> Content;
};

If in the future we need to add new sections with some new content - we can use a StringSaver NewSectionsContents
(i.e. it can be a field of the class Object) which would be responsible for holding the corresponding buffers (which are not owned by the input object)).

Looks like in this case the code would be a tiny bit cleaner / less verbose.

llvm/tools/llvm-objcopy/wasm/Writer.h
32 ↗(On Diff #233408)

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.

Thanks for all your input!
Who else needs to review this? Can you say explicitly, so we can keep this from becoming one of those reviews that hangs in limbo for a long time without any clear path forward?

I'm mostly just interested in getting someone with wasm experience to review this, to make sure there are no wasm-specific details that we might have missed. I don't really have time unfortunately to try to get to grips with the file format myself, otherwise I'd be happy to.

Also, obviously @alexshap's comments should be addressed.

sbc100 accepted this revision.Dec 13 2019, 9:28 AM

Looks fine from a wasm perspective.

This is just the very first step towards a full wasm objcopy but its look like a good start to me. Modulo a few comments inline.

llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test
42 ↗(On Diff #231943)

I feel like for yaml inputs like this should be the verbatim result of compiling some simple input file (either from .ll or .s) and as such should not be modified in place. Instead we should include instructions on how to regenerate it periodically if/when the yaml format changes.

For yaml output checking I agree there could be argument made for trimming since we often only check for a tiny subset.

llvm/test/tools/llvm-objcopy/wasm/basic-copy.test
13 ↗(On Diff #233408)

How was this yaml file generated? Is it the same object as in the other test? Should we instead commit this is a separate input file under: llvm/test/tools/llvm-objcopy/wasm/Input/. At least you should document how to regenerate these inputs.

(I you do you can remove one level of # from these test files too)

llvm/tools/llvm-objcopy/wasm/Writer.cpp
30 ↗(On Diff #233408)

Its section type an LEB, or a single byte? You use +1 below we means this should probably be a single byte write, no?

llvm/tools/llvm-objcopy/wasm/Writer.h
32 ↗(On Diff #233408)

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?

This revision is now accepted and ready to land.Dec 13 2019, 9:28 AM
llvm/tools/llvm-objcopy/wasm/Writer.h
32 ↗(On Diff #233408)

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.

jhenderson added inline comments.
llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test
42 ↗(On Diff #231943)

This sounds like a philosophical difference to me. I'd like to keep our test inputs to the bare minimum that actually test what we care about. This is the approach we take in the ELF objcopy testing, and indeed in most other newer tests on other tools like llvm-readobj (@grimar in particular has gone to a lot of effort to help improve those tests recently along these sort of lines).

If we start going down the approach of just blindly taking whatever obj2yaml produces for an input, many of the tests will be testing things that aren't really anything to do with their purpose.

That all being said, there may be a place for additional tests that do the round-trip obj2yaml, llvm-objcopy, yaml2obj testing, but I don't think these should be the general rule. In this case, I'd actually start from a .ll or .s file and use llvm-mc or whatever to turn them into an object. That way, there is no risk of the YAML and original source drifting apart.

grimar added inline comments.Dec 16 2019, 2:14 AM
llvm/test/tools/llvm-objcopy/wasm/basic-archive-copy.test
42 ↗(On Diff #231943)

I'd like to keep our test inputs to the bare minimum that actually test what we care about. This is the approach we take in the ELF objcopy testing, and indeed in most other newer tests on other tools like llvm-readobj

+1. If we use YAML it makes a lot of sense to keep it minimal. It is much easier to mantain, to extend and add new test cases.

If we start going down the approach of just blindly taking whatever obj2yaml produces for an input, many of the tests will be testing things that aren't really anything to do with their purpose.

Yes, and we saw that many times. For example, having an excessive "AddressAlign" flag might hide the possibility to produce a section that triggers a data type misalignment exception on some platforms for a few sections. It was fixed in llvm-readobj for versioning sections recently. If we did not try to use the minimal YAML descriptions, we would never caught such bugs.
The less YAML instructions we have - the more chances we test exactly what we want to test and not something else.

dschuff updated this revision to Diff 234098.Dec 16 2019, 10:17 AM
dschuff marked 3 inline comments as done.Dec 16 2019, 10:17 AM
  • Remove OwnedContents for now, and give Writer::SectionHeader its correct name
llvm/tools/llvm-objcopy/wasm/Object.h
21 ↗(On Diff #233408)

This interface (owning the contents) is actually used in D70970, but not here; I'll move it to the next review.
For now I kept the naming ("contentsRef") the same. in the next review we can discuss the current approach (which is meant for moving the contents) vs. your suggested approach which would copy them.

llvm/tools/llvm-objcopy/wasm/Writer.cpp
30 ↗(On Diff #233408)

You're right, it's a single byte in the spec; it doesn't make any difference if it's less than 128 and I think we are not 100% consistent in the tools code. But I fixed it here.

llvm/tools/llvm-objcopy/wasm/Writer.h
32 ↗(On Diff #233408)

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.

Thanks for your patience, a few more comments.

llvm/test/tools/llvm-objcopy/wasm/basic-copy.test
6 ↗(On Diff #234098)

-o ?

llvm/tools/llvm-objcopy/wasm/Object.h
41 ↗(On Diff #234098)

since WasmObjectHeader is public i would convert this class into a struct as well / get rid of boilerplate code (getSections/addSections etc.)
So basically I'd keep it as simple & clear as possible, and if we need to add complexity we'll do it when/where it's required (and there it'll be easier to understand / judge what exactly is necessary).

21 ↗(On Diff #233408)

well, for now it seems like it should be a struct (e.g. as in the comment above), because otherwise this looks a bit strange imo. (+ it looks like this "using" and the setter method would not be necessary).

dschuff marked an inline comment as done.Dec 16 2019, 5:35 PM
dschuff added inline comments.
llvm/tools/llvm-objcopy/wasm/Object.h
41 ↗(On Diff #234098)

I understand the concern here (and above) but I'd rather not completely refactor this class now, only to completely rewrite it back tomorrow (since I intend to finish up D70970 as soon as this patch lands). I don't think it makes sense to get rid of addSections given that we'll have removeSections soon, and removeSections is slightly nontrivial. I think the more useful question though is how to handle owned and non-owned section data, since in the next change we'll need to have both. I went ahead and rewrote Section for now, and I made the Object own the data (instead of the section) in the next patch so you can see what you think of that.

dschuff updated this revision to Diff 234203.Dec 16 2019, 5:36 PM
  • Simplify Section to a struct
dschuff marked an inline comment as done.Dec 16 2019, 5:38 PM
dschuff added inline comments.
llvm/test/tools/llvm-objcopy/wasm/basic-copy.test
6 ↗(On Diff #234098)

obj2yaml does not support the -o option.

llvm/tools/llvm-objcopy/wasm/Object.cpp
22 ↗(On Diff #234203)

Sections.insert(Sections.end(), NewSections.begin(), NewSections.end());

llvm/tools/llvm-objcopy/wasm/Object.h
22 ↗(On Diff #234203)

"public:" is unnecessary

llvm/tools/llvm-objcopy/wasm/Reader.cpp
24 ↗(On Diff #234203)

Sections.push_back({WS.Type(), WS.Name, WS.Content});

llvm/tools/llvm-objcopy/wasm/Writer.h
32 ↗(On Diff #233408)

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

llvm/tools/llvm-objcopy/wasm/Writer.h
32 ↗(On Diff #233408)

+ 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)

dschuff updated this revision to Diff 234406.Dec 17 2019, 3:32 PM
dschuff marked 3 inline comments as done.
  • Split header generation into separate function
dschuff marked an inline comment as done.Dec 17 2019, 3:36 PM
dschuff added inline comments.
llvm/tools/llvm-objcopy/wasm/Writer.h
32 ↗(On Diff #233408)

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.

okay, thank you again for the patience, overall this looks cleaner to me

llvm/tools/llvm-objcopy/wasm/Reader.cpp
26 ↗(On Diff #234406)

sorry, I didn't notice this before - but this is one reason why I wasn't a big fan of the proposed interface,
adding a section (or sections) always involves a copy.
For example, if vector<Section> Sections was just a public field of Object
(or Object was a struct (it still can have a method removeSections if necessary))
you would simply do the following:

Obj.Sections.reserve(WasmObj.sections().size());
for (SectionRef Sec : WasobObj.sections()) {
     Sections.push_back(... );
}

Frankly speaking it still feels like addSections(...) doesn't buy us much. (and the same is true for getSections)

(+ please, apply clang-format --style=llvm to the diff (if not yet))

dschuff updated this revision to Diff 239707.Jan 22 2020, 2:47 PM
  • Make Sections a public member of Object; remove getSections/addSections
dschuff updated this revision to Diff 239708.Jan 22 2020, 2:48 PM
  • fix clang-format

OK, made Sections a public member of Object.

I think all the feedback has now been addressed.

Unit tests: pass. 62116 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dschuff updated this revision to Diff 239721.Jan 22 2020, 3:23 PM
  • Fix header guard name

Unit tests: pass. 62116 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 4 warnings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Hm, the bot seems to be using the wrong version of the patch.

Unit tests: pass. 62116 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

I've given this another quick once over, and made a few small comments. I'll try to take a more in-depth look tomorrow or early next week.

llvm/tools/llvm-objcopy/wasm/Writer.cpp
24–25 ↗(On Diff #239721)

Missing trailing full stops on these two lines.

31 ↗(On Diff #239721)

// static?

llvm/tools/llvm-objcopy/wasm/Writer.h
32–33 ↗(On Diff #239721)

Not a big deal, but any particular reason you've called this generateSectionHeader instead of the shorter createSectionHender?

dschuff updated this revision to Diff 239921.Jan 23 2020, 9:04 AM
dschuff marked 3 inline comments as done.
  • Rename generateSectionHeader, clarify comments
llvm/tools/llvm-objcopy/wasm/Writer.cpp
24–25 ↗(On Diff #239721)

done (but these are intentionally not complete sentences).

31 ↗(On Diff #239721)

This is just the common annotation indicating that generateSectionHeader is a static method. I added a blank line to make it more clear that it goes with the method definition rather than the block comment above.

llvm/tools/llvm-objcopy/wasm/Writer.h
32–33 ↗(On Diff #239721)

No particular reason; habit from working on code generators I suppose. I changed it.

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jhenderson added inline comments.Jan 27 2020, 6:54 AM
llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
23 ↗(On Diff #239921)

Is this using needed? You're already inside the wasm namespace.

43 ↗(On Diff #239921)

Strictly speaking, basic copying isn't a flag! Perhaps simply "no flags are supported yet" or possibly "no flags are supported yet. Only basic copying is allowed".

llvm/tools/llvm-objcopy/wasm/Writer.cpp
31 ↗(On Diff #239721)

If it's common practice within other areas of WASM code, then that's fine, but I wasn't aware that it's a wider thing. To me, it initially looked like you'd just accidentally commented it out or something similar.

22–30 ↗(On Diff #239921)

I wonder whether this comment would make more sense in the header?

76 ↗(On Diff #239921)

A blank line before this and the similar comment above would nicely break this function up, I think.

dschuff updated this revision to Diff 240702.Jan 27 2020, 3:27 PM
dschuff marked 5 inline comments as done.
  • Rename generateSectionHeader, clarify comments
llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
43 ↗(On Diff #239921)

Done as you suggested, (but I added a full stop at the end ;-).

llvm/tools/llvm-objcopy/wasm/Writer.cpp
31 ↗(On Diff #239721)

Looking again, it doesn't seem to be as common in LLVM as in some other codebases I've used; I'll just remove it here.

22–30 ↗(On Diff #239921)

Yeah that makes sense; moved the comment.

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dschuff updated this revision to Diff 240713.Jan 27 2020, 4:04 PM
  • Fix clang-tidy warning

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dschuff updated this revision to Diff 240736.Jan 27 2020, 5:07 PM
  • remove stray empty line

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jhenderson accepted this revision.Jan 28 2020, 1:13 AM

LGTM, with two nits.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp
43 ↗(On Diff #239921)

Actually, for error messages, we usually don't end with a full stop :-)

The style we try to canonicalise on in the other binutils is lower-case first letter of first sentence, and no trailing full stops.

Don't ask me why that is the style, mind you...

llvm/tools/llvm-objcopy/wasm/Writer.h
32–40 ↗(On Diff #240736)

We're a bit hit-and-miss on this, but it's probably good practice to use doxygen-style comments (i.e. '///') for things like this in headers.

dschuff updated this revision to Diff 240911.Jan 28 2020, 9:38 AM
dschuff marked 2 inline comments as done.
  • remove period, use doxygen comments
This revision was automatically updated to reflect the committed changes.

Unit tests: fail. 62112 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

kamaub added a subscriber: kamaub.EditedJan 29 2020, 7:18 AM

@dschuff please note this patch causes build failures for PowerPC big endian buildbots with commit rGa928d127a52a
If the solution isn't a simple fix please revert the commit so we may continue testing the rest of the repository

D72723 may show up with some failures in between but here but this has since been resolved
i've attached the builds that broke and the lastest build from those bots:

clang-ppc64be-linux:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43577
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43643

clang-ppc64be-linux-multistage:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23316
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23336

clang-ppc64be-linux-lnt:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34687
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34719

I won't have time to look at it today; i'll just revert and take a look tomorrow.

Relanded with a fix as rGf2af0607000c
I don't have a BE machine to test on, but I'm *pretty* sure it works :)

I won't have time to look at it today; i'll just revert and take a look tomorrow.

Thank you, the buildbots are green again, best of luck addressing the failure.