• update test

If @aardappel and @dschuff are ok with this change I'll defer to them.

Reopening as the alternative solution had issues.

This change is looking nice!

Thanks. lgtm

In D55381#1321857, @amontanez wrote:

Apologies, I'll keep a closer eye on that in the future. This can be resolve by doing what ELFArch has done. This requires adding a typedef to TBEHandler.h, changing the declaration of NeededLibs to use the new typedef, and then modifying the ELFYAMLTest.cpp to use something like

LLVM_YAML_STRONG_TYPEDEF(ELFNeededList, ELFNeededMapper)

That should allow us to keep the flow mapping.

Attempted fix in https://reviews.llvm.org/D55381

We are seeing a failure of YAMLWritesNoTBESyms on the WebAssembly waterfall due to this change:

lgtm % comment

Since event types can't have return values (they are always no_return right?) should we split out "parseParamList" and have them look like this?

There is a presensent for the "emit" functions in the streamer modifying symbols. For example:

IIUC this change is NFC so you can mark it as such in the title?

Should the first word of the description be WasmSymbol

In D54873#1317523, @aheejin wrote:

Yes in that case it's represented like sigindex of functions.

- Type:            FUNCTION                                                    
  FunctionTypes:   [ 0, 0 ]                                                    
- Type:            EVENT                                                       
  EventTypes:      [ 1 ]                   // here                                        
  Events:                                                 
    - Index:           0                                                       
      Attribute:       0

In D54873#1317509, @aheejin wrote:

In D54873#1316963, @sbc100 wrote:

And why do we need this for specifying events themselves and for importing events?

I mean event imports need to specify their "type" and non-imported events also need to specify their "type". And in both cases the type needs to include the signature. So I think we should have a unified "eventType" that includes the signature index.

I can drop this CL, but I still don't understand what you meant by this part. Currently imported exception's sigindex is handled fine. The below is obj2yaml's result if I don't define the __cpp_exception symbol:

- Type:            IMPORT                                                      
  Imports:                                                                     
    ...                                    
    - Module:          env                                                     
      Field:           __cpp_exception                                         
      Kind:            EVENT                                                   
      SigIndex:        1                                                       
      EventAttribute:  0

As you see in WasmImport definition in this CL, SigIndex is now out of union and every type can set it optionally, so there's no problem for events having sigindex there.

struct WasmImport {                                                              
  StringRef Module;                                                              
  StringRef Field;                                                               
  uint8_t Kind;                                                                  
  uint32_t SigIndex;                                                             
  union {                                                                        
    WasmGlobalType Global;                                                       
    WasmTable Table;                                                             
    WasmLimits Memory;                                                           
    WasmEventType Event;                                                         
  };                                                                             
};

In D54873#1315302, @aheejin wrote:

In D54873#1315295, @sbc100 wrote:

In D54873#1315289, @aheejin wrote:

Ping. So the decision points are

1. Whether SigIndex should be in or out of WasmEventType?
   1. If SigIndex should be out of WasmEventType, should there even be WasmEventType anymore? Because then there's only Attribute left.
2. It seems to be people don't like the term Attribute. Should we change it to Kind or something? We can change the spec text, because it's not set in stone or anything.

Seems like we should keep the two member WasmEventType. We need this for specifying the events themselves, and also for importing events. It seems like this matches WasmGlobalType,

I'm not really sure what this means. Do you mean we should keep SigIndex as a part of WasmEventType and drop this CL?

In D54873#1315289, @aheejin wrote:

Ping. So the decision points are

1. Whether SigIndex should be in or out of WasmEventType?
   1. If SigIndex should be out of WasmEventType, should there even be WasmEventType anymore? Because then there's only Attribute left.
2. It seems to be people don't like the term Attribute. Should we change it to Kind or something? We can change the spec text, because it's not set in stone or anything.

• feedback

In D54924#1310780, @aheejin wrote:

D44024 is not simpler; actually it is more complicated because it checks more things than this, such as custom sections and whether the function section matches with the code section. If you think we should also include custom section checking in this CL, why don't we just land D44024?

In D54924#1310734, @aheejin wrote:

I see.. so why wasn't it landed? It says it was stalled because we couldn't decide whether or how to check custom section orderings and orderings between custom section vs. normal sections. Do we now have consensus on that? If so landing that CL makes more sense. If not, do you think now is a good time to re-discuss it? Or would landing this CL first and decide what to do with custom sections later makes sense?

The main motivation for this CL was, I had to delete code that was previously checking relative section orders in yaml2wasm because the previous code only checked if the section codes were monotonically increasing and the new event section had code 12, and you suggested we should reinstate it in a separate CL.

lgtm % nits and the question of whether we leave SigIndex as part of the EventType in wasm.h.

For reference there was a previous CL that tries to do something similar: https://reviews.llvm.org/D44024.

• sort

• feedback

In D54758#1308253, @ruiu wrote:

In D54758#1307205, @kristina wrote:

I think some redundant includes should definitely go (IWYU). Regarding more granular using declarations, honestly it's up to @ruiu, Clang and LLDB handle it by aliasing some of LLVM's common classes in their respective namespaces, maybe that could work for LLD too? In fact I'm guessing that's the purpose of lld/Common/LLVM.h, you could take a similar approach for Wasm specific stuff and limit the using statements to your namespace.

Your guess is correct; lld/Common/LLVM.h is to using some identifiers so that they can be referred without namespace specifiers. In fact, I think it is fine to add Wasm* names to that file because it is obvious that they will never conflict with other names because of Wasm prefix. Some names are ambiguous, such as llvm::object::Archive, and it is better to refer them using a fully qualified name, as we have other classes with confusing names (e.g. lld::*::ArchiveFile).

I general I think this is good change. I think there will always be point in the abstraction where we start prefering "spec" terms over "llvm" terms.

In D54758#1304845, @aheejin wrote:

I think what @dschuff said in this comment was not removing all using from headers, but removing using namespace from headers.. no? We are not using using namespace in header files already.

In D54683#1303746, @dschuff wrote:

LLVM doesn't actually have an IWYU guideline though: https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible ... but also not a "dont IWYU".

Also this general problem is why Chrome has a rule against using namespace in header files. I like that rule because you can always tell by looking in the cpp file which namespaces are in scope.
If we do the same, then using directive in a cpp file can be removed if the file still compiles after removing it because we know it's not coming from somewhere else.

Also even if we adopt that convention for the wasm backend we might have to make an exception for using namespace llvm because it's so ubiquitous.

In D54661#1303664, @dschuff wrote:

Not sure I am parsing your commit message right. So we fold if DSO local, and not otherwise?

Yes, this should have no effect unless we are compiling with -fPIC and have non-DSO-local symbols.

lgtm %nits

Hmm, I guess at some point in the past InputFunctions and InputGlobals were not empty at the start of this function.

I'm not sure how you created this change, but I think using statements are like IWYU in that you don't want remove any line that can be removed and have the file still compile. i.e. you don't want to rely on recursively included using statements.

get parts of address-offsets.ll passing

Nice! I wish we could do with without the lexer change and with a more asm-linux syntax.. but I guess that time for bikeshedding that is over.

@dschuff can you take a look at the WebAssemblyFastISel.cpp part? I want to make sure I'm on the right track.

cleanup

In D54571#1299575, @xbolva00 wrote:

Sorry for little offtopic at first.

WebAssembly folks, can you check this warning:

WebAssemblyDisassembler.cpp:125:11: warning: comparison is always false due to limited range of data type [-Wtype-limits]

if (Opc < 0)

GCC 8.2

• feedback
• clang-format

The diff algorithms seem to make a right mess of this, but this change basically splits out setConfigs and checkOptions and nothing more