[obj2yaml] - Report a error when unable to resolve a sh_link reference properly.

[grimar]

Summary

Because of a bug we did not report a error in the case
shown in the test. With this patch we do.

[MaskRay]

tools/obj2yaml/elf2yaml.cpp

380

So this prepends a message to the existing toString(LinkSection.takeError()).

LGTM, but let's see what james wants to say.

[grimar]

tools/obj2yaml/elf2yaml.cpp

380

So this prepends a message to the existing toString(LinkSection.takeError()).

Yeah, but the initial probem was in double takeError() call. As a result no error was reported at all.

[grimar]

James, do you have anything to add?

[jhenderson]

In D65214#1602276, @grimar wrote:

James, do you have anything to add?

Sorry, it's been a busy couple of days, and I ran out of time to look at this yesterday. Code change looks fine. Just a comment about the test.

test/tools/obj2yaml/elf-shlink.yaml

1

Should we have a test case to show that it can handle the elf-shlink value otherwise, or does that already exist?

If it already exists (or for some reason don't want it), perhaps you should rename this file elf-shlink-invalid.yaml.

[grimar]

In D65214#1602306, @jhenderson wrote:

In D65214#1602276, @grimar wrote:

James, do you have anything to add?

Sorry, it's been a busy couple of days, and I ran out of time to look at this yesterday. Code change looks fine. Just a comment about the test.

No worries at all. I shouldn't have to ping you first of all, since LLVM policy suggest to do this once in a ~week.
(I just supposed you probably missed this comment for some reason, but there is definetely no need to rush in this or another patches I post).
Thanks for looking!

[grimar]

• Addressed review comments.

test/tools/obj2yaml/elf-shlink.yaml

1

Should we have a test case to show that it can handle the elf-shlink value otherwise, or does that already exist?

We have a lot of tests in tools/obj2yaml that do something like: Link: .dynsym. I.e. refer to an existent section by name.
But probably this is kind of implicit and not complete way.

So, I think you're right anf it might worth to test the behavior explicitly here. I added the tests.

[jhenderson]

LGTM.

[grimar]

Revision: 367203

test/tools/obj2yaml/elf-shlink.yaml

+## Check obj2yaml reports a proper error when unable to resolve
+## a sh_link reference in a regular section.
+## (We had a bug: the error was silently ignored).
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: not obj2yaml %t.o 2>&1 | FileCheck %s -DFILE=%t.o
+
+## CHECK: Error reading file: [[FILE]]: unable to resolve sh_link reference in section '.test': invalid section index: 42
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .test
+    Type: SHT_PROGBITS
+    Link: 42

tools/obj2yaml/elf2yaml.cpp

Error ELFDumper<ELFT>::dumpCommonSection(const Elf_Shdr *Shdr,
 
   auto NameOrErr = getUniquedSectionName(Shdr);
   if (!NameOrErr)
     return NameOrErr.takeError();
   S.Name = NameOrErr.get();
 
   if (Shdr->sh_link != ELF::SHN_UNDEF) {
     auto LinkSection = Obj.getSection(Shdr->sh_link);
-    if (LinkSection.takeError())
-      return LinkSection.takeError();
+    if (!LinkSection)
+      return make_error<StringError>(
+          "unable to resolve sh_link reference in section '" + S.Name +
+              "': " + toString(LinkSection.takeError()),
+          inconvertibleErrorCode());
+
     NameOrErr = getUniquedSectionName(*LinkSection);
     if (!NameOrErr)
       return NameOrErr.takeError();
     S.Link = NameOrErr.get();
   }
 
   return Error::success();
 }