[WebAssembly][libObject] Avoid re-use of Section object during parsing

[sbc100]

Summary

The re-use of this struct across iterations of the loop was causing
fields (specifically Name) to be incorrectly shared between multiple
sections.

[jhenderson]

The test has changed quite a bit more than I'd expect. What's the reasoning behind that?

[sbc100]

In D108984#2974366, @jhenderson wrote:

The test has changed quite a bit more than I'd expect. What's the reasoning behind that?

This change means that the this test acts a regression test for the bug. In order for the bug to show itself we have to put a custom section before a builtin section. This is because only custom sections have names and the bug is that the Name field from a custom section was getting applied to all builtin sections that follow it.

If you prefer I can leave this test as is but write a specific regression test in llvm/test/Object instead?

[dschuff]

I guess another option on the test would be to just keep the same sections that are in this test now, but still add foo at the front. The existing tests can stay the same and we could add one that covers this case (or we could switch one of the existing tests to use foo instead of producers if that would cover both cases, so we don't need to increase the total number of tests).

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

17

The intent here was to test that the known sections are gone (because the custom section was after the known sections). But now it doesn't test that, because the custom section is after the known sections.

[sbc100]

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

17

Oh in that case I can just CHECK-NEXT: ... to ensure this is the one and only section. That seems strictly more clear.

[sbc100]

Better test

[jhenderson]

If you prefer I can leave this test as is but write a specific regression test in llvm/test/Object instead?

I personally would generally prefer a gtest unit test for library-level changes, as I find the actual testing can be more specific, and less prone to rotting so that the test no longer covers the thing it's supposed to cover. However, I'm not a wasm developer, so I'm not going to ask for it, as there are probably norms there that are better followed.

Anyway, LGTM, but please wait for @dschuff.

[dschuff]

AFAIK we don't have gtest tests for wasm's lib/Object code so I don't want to block this little fix on creating a whole new set of tests. This change to the existing tests looks ok to me.
But I am interested in gtest unit tests more, because I like that style of testing generally. e.g. gor the file-based tests we have YAML as a convenient way to create test input, but it seems like for gtest tests you'd have to duplicate or write some custom test-harness code to generate inputs. Do you know if there are existing unit tests for lib/Object code, or know of any best practices for testing that library?

[jhenderson]

In D108984#2976999, @dschuff wrote:

AFAIK we don't have gtest tests for wasm's lib/Object code so I don't want to block this little fix on creating a whole new set of tests. This change to the existing tests looks ok to me.
But I am interested in gtest unit tests more, because I like that style of testing generally. e.g. gor the file-based tests we have YAML as a convenient way to create test input, but it seems like for gtest tests you'd have to duplicate or write some custom test-harness code to generate inputs. Do you know if there are existing unit tests for lib/Object code, or know of any best practices for testing that library?

llvm/unittests/Object contains unittests for the Object library. Mostly, they're testing the easier to test API, but there are some, such as ELFObjectFileTest.cpp and MinidumpTest.cpp, which do similar things to what you might need to do to get wasm libObject tests sorted.

[sbc100]

In D108984#2978931, @jhenderson wrote:

In D108984#2976999, @dschuff wrote:

AFAIK we don't have gtest tests for wasm's lib/Object code so I don't want to block this little fix on creating a whole new set of tests. This change to the existing tests looks ok to me.
But I am interested in gtest unit tests more, because I like that style of testing generally. e.g. gor the file-based tests we have YAML as a convenient way to create test input, but it seems like for gtest tests you'd have to duplicate or write some custom test-harness code to generate inputs. Do you know if there are existing unit tests for lib/Object code, or know of any best practices for testing that library?

llvm/unittests/Object contains unittests for the Object library. Mostly, they're testing the easier to test API, but there are some, such as ELFObjectFileTest.cpp and MinidumpTest.cpp, which do similar things to what you might need to do to get wasm libObject tests sorted.

Interesting, it looks like they also use yaml2obj to generate test input in the unit tests:
https://github.com/llvm/llvm-project/blob/dfb7518df18ad420984f965ef5ccfb4e126d839b/llvm/unittests/Object/ELFObjectFileTest.cpp#L318-L331

For now, I think I'm OK with leaving the testing here in the objcopy tests since that bug in question only effects objcopy (no other tools seem to care about the name of non-custom sections).

llvm/lib/Object/WasmObjectFile.cpp

WasmObjectFile::WasmObjectFile(MemoryBufferRef Buffer, Error &Err)
   Header.Version = readUint32(Ctx);
   if (Header.Version != wasm::WasmVersion) {
     Err = make_error<StringError>("invalid version number: " +
                                       Twine(Header.Version),
                                   object_error::parse_failed);
     return;
   }
 
-  WasmSection Sec;
   WasmSectionOrderChecker Checker;
   while (Ctx.Ptr < Ctx.End) {
+    WasmSection Sec;
     if ((Err = readSection(Sec, Ctx, Checker)))
       return;
     if ((Err = parseSection(Sec)))
       return;
 
     Sections.push_back(Sec);
   }
 }

llvm/test/tools/llvm-objcopy/wasm/basic-only-section.test

 ## Test --only-section.
 # RUN: yaml2obj %s -o %t
-# RUN: llvm-objcopy --only-section=producers %t %t2
+# RUN: llvm-objcopy --only-section=foo %t %t2
 # RUN: obj2yaml %t2 | FileCheck --implicit-check-not TYPE --implicit-check-not linking %s
 
 ## Test that it's the same with only-section + keep-section (for the same section).
-# RUN: llvm-objcopy --only-section=producers --keep-section=producers %t %t2
+# RUN: llvm-objcopy --only-section=foo --keep-section=foo %t %t2
 # RUN: obj2yaml %t2 | FileCheck --implicit-check-not TYPE --implicit-check-not linking %s
 
 ## Also test that only-section overrides remove-section.
-# RUN: llvm-objcopy --only-section=producers --remove-section=producers %t %t2
+# RUN: llvm-objcopy --only-section=foo --remove-section=foo %t %t2
 # RUN: obj2yaml %t2 | FileCheck --implicit-check-not linking %s
 
-## This file has both known and custom sections. Check that only the producers section is left.
+## This file has both known and custom sections. Check that only the foo section is left.
 # CHECK:      Sections:
 # CHECK-NEXT:   - Type: CUSTOM
-# CHECK-NEXT:     Name: producers
-# CHECK-NEXT:     Tools:
+# CHECK-NEXT:     Name: foo
+# CHECK-NEXT:     Payload: DEADBEEF
 
 ## Test that only-section + keep-section keeps both sections.
-# RUN: llvm-objcopy --only-section=producers --keep-section=linking %t %t2
+# RUN: llvm-objcopy --only-section=foo --keep-section=linking %t %t2
 # RUN: obj2yaml %t2 | FileCheck --implicit-check-not=TYPE --check-prefix=KEEP %s
+# KEEP: Name: foo
 # KEEP: Name: linking
-# KEEP: Name: producers
 
 --- !WASM
 FileHeader:
   Version: 0x00000001
 Sections:
+  - Type: CUSTOM
+    Name: foo
+    Payload: DEADBEEF
   - Type: TYPE
     Signatures:
       - Index: 0
         ParamTypes:
           - I32
         ReturnTypes:
           - F32
   - Type: CUSTOM
     Name: linking
     Version: 2
-  - Type: CUSTOM
-    Name: producers
-    Tools:
-      - Name:   clang
-        Version: 9.0.0