Gracefully handle empty .drectve sections

[kastiglione]

Summary

Running llvm-readobj -coff-directives msvcrt.lib resulted in this error:

Invalid data was encountered while parsing the file

This happened because some of the object files in the archive have empty
.drectve sections. These empty sections result in a parse_failed error being
returned from COFFObjectFile::getSectionContents(), which in turn caused
llvm-readobj to stop. With this change, getSectionContents now returns
success, and like before the resulting array is empty.

[kastiglione]

Move size check before name check

[smeenai]

Empty sections causing a parse failure strikes me as slightly odd, but I'm guessing there's a good rationale for it. @compnerd?

[ruiu]

I feel like we should fix COFFObjectFile::getSectionContents to not fail on empty sections instead of avoid calling it on empty sections, as I naturally expect that getSectionContents returns an empty StringRef for an empty section.

[smeenai]

In D32652#741088, @ruiu wrote:

I feel like we should fix COFFObjectFile::getSectionContents to not fail on empty sections instead of avoid calling it on empty sections, as I naturally expect that getSectionContents returns an empty StringRef for an empty section.

That was my intuition as well, but I wasn't sure if there was a reason for the existing behavior/callers that depend on that behavior.

[majnemer]

Testcase?

[kastiglione]

Added a test covering the desired behavior.

[ruiu]

test/Object/coff-empty-drectve.test

3 ↗

(On Diff #97389)

You want to add {{$}} at end of this line to make sure that Directive(s): is not followed by any other characters.

4 ↗

(On Diff #97389)

Remove trailing empty line.

[kastiglione]

Constrain FileCheck match with {{$}} and remove extra newline.

[ruiu]

LGTM, but please wait until tomorrow to give other people a chance to take a look.

test/Object/Inputs/COFF/empty-drectve.yaml

1–14 ↗

(On Diff #97391)

You can merge this file with coff-empty-drectve.test file.

test/Object/coff-empty-drectve.test

1 ↗

(On Diff #97391)

To do that you want to change this to

yaml2obj %s | llvm-readobj ...

3 ↗

(On Diff #97391)

And make this line a YAML comment.

[kastiglione]

please wait until tomorrow

I don't have commit access anyway :)

test/Object/Inputs/COFF/empty-drectve.yaml

1–14 ↗

(On Diff #97391)

I was going to write the test this way originally, but the handful of tests I looked at in test/Object mostly all used a separate input file, it seemed like this was the convention. I'm happy to merge these two files.

[ruiu]

I do not have a strong preference if that's a local convention here.

[kastiglione]

@ruiu would you be willing to commit this for me? I'm happy to change the test to be single file too.

llvm/trunk/lib/Object/COFFObjectFile.cpp

 }
 
 std::error_code
 COFFObjectFile::getSectionContents(const coff_section *Sec,
                                    ArrayRef<uint8_t> &Res) const {
   // In COFF, a virtual section won't have any in-file
   // content, so the file pointer to the content will be zero.
   if (Sec->PointerToRawData == 0)
-    return object_error::parse_failed;
+    return std::error_code();
   // The only thing that we need to verify is that the contents is contained
   // within the file bounds. We don't need to make sure it doesn't cover other
   // data, as there's nothing that says that is not allowed.
   uintptr_t ConStart = uintptr_t(base()) + Sec->PointerToRawData;
   uint32_t SectionSize = getSectionSize(Sec);
   if (checkOffset(Data, ConStart, SectionSize))
     return object_error::parse_failed;
   Res = makeArrayRef(reinterpret_cast<const uint8_t *>(ConStart), SectionSize);

llvm/trunk/test/Object/Inputs/COFF/empty-drectve.yaml

+--- !COFF
+header:
+  Machine:           IMAGE_FILE_MACHINE_I386
+sections:
+  - Name:            .drectve
+    Characteristics: [ IMAGE_SCN_LNK_INFO, IMAGE_SCN_LNK_REMOVE ]
+    SectionData:     ''
+symbols:
+  - Name:            .drectve
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC

llvm/trunk/test/Object/coff-empty-drectve.test

+RUN: yaml2obj %p/Inputs/COFF/empty-drectve.yaml | llvm-readobj -coff-directives - | FileCheck %s
+
+CHECK: Directive(s): {{$}}