[WebAssembly] Add WASM_SEC_LAST_KNOWN to BinaryFormat section types list [NFC]

[dschuff]

Summary

There are 3 places where we were using WASM_SEC_TAG as the "last" known
section type, which requires updating (or leaves a bug) when a new known
section type is added. Instead add a "last type" to the enum for this
purpose.

[aheejin]

llvm/include/llvm/BinaryFormat/Wasm.h

256

• Does this pass clang-format? Because we don't have a space after ,.
• 14 is not actually the last section known, because we don't have a section whose section code is 14. How about doing something like

WAS_SEC_LAST_KNOWN = WASM_SEC_TAG;

?

llvm/lib/ObjCopy/wasm/WasmReader.cpp

32–35

Why was this removed? It doesn't look like it's related to the last section thing.. It's just explaning custom sections have names already.

[dschuff]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

32–35

Checking against WASM_SEC_LAST_KNOWN ensures that there can't be a new type of section that we don't explicitly handle here (because no types of known sections are explicitly handled or mentioned); any section known to sectionTypeToString will work.

[dschuff]

• review comments

[dschuff]

llvm/include/llvm/BinaryFormat/Wasm.h

256

ah, no it doesnt pass clang-format.
Also, agreed that LAST_KNOWN should match TAG rather than the one past-the-end. Fixed.

[dschuff]

• expand comment

[aheejin]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

32–35

I'm not sure if I understand. Is this related to WASM_SEC_TAG -> WASM_LAST_SEC_KNOWN change in this PR? Or it's just an unrelated drive-by fix?

Checking against WASM_SEC_LAST_KNOWN ensures that there can't be a new type of section that we don't explicitly handle here (because no types of known sections are explicitly handled or mentioned);

I'm not sure what this means..

If the section type is CUSTOM, it has a name already.

Why is this deleted too? How is the custom section related to WASM_SEC_LAST_KNOWN?

[dschuff]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

32–35

It is indirectly related.
The purpose of this block of code is to fill in the Name field of the WasmSection object for known sections (as the comment on line 27 explains). Suppose we add a new type of known section, WASM_SEC_NEW, and we update sectionTypeToString but forget to update this file. Before this CL, the behavior would be as described in the comment I'm deleting; this code would silently fail to give that section a name, which would mean it couldn't be selected by llvm-objdump. After this CL, everything would work, without needing any updates to this file (and if we forgot to update sectionTypeToString it would assert when encountering an unknown section type). So this comment was just to describe the somewhat surprising failure mode, and isn't necessary anymore.

[aheejin]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

32–35

I see. My original question was why

If the section type is CUSTOM, it has a name already.

is related to this CL, but I see you moved that comment to elsewhere, so it hasn't really changed..

[dschuff]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

32–35

Ah yeah that bit of explanation is still valuable. Thanks!

lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp

if (section_id == llvm::wasm::WASM_SEC_CUSTOM) {
 
     if (payload_len < c.tell() - prev_offset)
       return false;
 
     uint32_t section_length = payload_len - (c.tell() - prev_offset);
     m_sect_infos.push_back(section_info{*offset_ptr + c.tell(), section_length,
                                         section_id, *sect_name});
     *offset_ptr += (c.tell() + section_length);
-  } else if (section_id <= llvm::wasm::WASM_SEC_TAG) {
+  } else if (section_id <= llvm::wasm::WASM_SEC_LAST_KNOWN) {
     m_sect_infos.push_back(section_info{*offset_ptr + c.tell(),
                                         static_cast<uint32_t>(payload_len),
                                         section_id, ConstString()});
     *offset_ptr += (c.tell() + payload_len);
   } else {
     // Invalid section id.
     return false;
   }

llvm/include/llvm/BinaryFormat/Wasm.h

enum : unsigned {
   WASM_SEC_MEMORY = 5,     // Memory attributes
   WASM_SEC_GLOBAL = 6,     // Global declarations
   WASM_SEC_EXPORT = 7,     // Exports
   WASM_SEC_START = 8,      // Start function declaration
   WASM_SEC_ELEM = 9,       // Elements section
   WASM_SEC_CODE = 10,      // Function bodies (code)
   WASM_SEC_DATA = 11,      // Data segments
   WASM_SEC_DATACOUNT = 12, // Data segment count
-  WASM_SEC_TAG = 13        // Tag declarations
+  WASM_SEC_TAG = 13,       // Tag declarations
+  WASM_SEC_LAST_KNOWN = WASM_SEC_TAG,
 };
 
 // Type immediate encodings used in various contexts.
 enum : unsigned {
   WASM_TYPE_I32 = 0x7F,
   WASM_TYPE_I64 = 0x7E,
   WASM_TYPE_F32 = 0x7D,
   WASM_TYPE_F64 = 0x7C,

llvm/lib/ObjCopy/wasm/WasmReader.cpp

Expected<std::unique_ptr<Object>> Reader::create() const {
   auto Obj = std::make_unique<Object>();
   Obj->Header = WasmObj.getHeader();
   std::vector<Section> Sections;
   Obj->Sections.reserve(WasmObj.getNumSections());
   for (const SectionRef &Sec : WasmObj.sections()) {
     const WasmSection &WS = WasmObj.getWasmSection(Sec);
     Obj->Sections.push_back(
         {static_cast<uint8_t>(WS.Type), WS.Name, WS.Content});
-    // Give known sections standard names to allow them to be selected.
+    // Give known sections standard names to allow them to be selected. (Custom
+    // sections already have their names filled in by the parser).
     Section &ReaderSec = Obj->Sections.back();
     if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
-        ReaderSec.SectionType <= WASM_SEC_TAG)
+        ReaderSec.SectionType <= WASM_SEC_LAST_KNOWN)
       ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
-    // If the section type is CUSTOM, it has a name already. If it's a new type
-    // of section that we don't explicitly handle here, it will have an empty
-    // name and objcopy won't be able to select it by name (e.g. for removal
-    // or dumping) but it will still be valid and able to be copied.
   }
   return std::move(Obj);
 }
 
 } // end namespace wasm
 } // end namespace objcopy
 } // end namespace llvm

llvm/lib/Object/WasmObjectFile.cpp

 }
 
 void WasmObjectFile::moveSectionNext(DataRefImpl &Sec) const { Sec.d.a++; }
 
 Expected<StringRef> WasmObjectFile::getSectionName(DataRefImpl Sec) const {
   const WasmSection &S = Sections[Sec.d.a];
   if (S.Type == wasm::WASM_SEC_CUSTOM)
     return S.Name;
-  if (S.Type > wasm::WASM_SEC_TAG)
+  if (S.Type > wasm::WASM_SEC_LAST_KNOWN)
     return createStringError(object_error::invalid_section_index, "");
   return wasm::sectionTypeToString(S.Type);
 }
 
 uint64_t WasmObjectFile::getSectionAddress(DataRefImpl Sec) const { return 0; }
 
 uint64_t WasmObjectFile::getSectionIndex(DataRefImpl Sec) const {
   return Sec.d.a;