This is an archive of the discontinued LLVM Phabricator instance.

[lib/ObjectYAML] - Improve and cleanup error reporting in ELFState<ELFT> class.
ClosedPublic

Authored by grimar on Sep 4 2019, 8:25 AM.

Details

Summary

The aim of this patch is to refactor how we handle and report error.

I suggest to use the same approach we use in LLD: delayed error reporting.
For that I introduced 'HasError' flag which triggers when we report an error.
Now we do not exit instantly on any error. The benefits are:

  1. There are no more 'exit(1)' calls in the library code.
  2. Code was simplified significantly in a few places.
  3. It is now possible to print multiple errors instead of only one.

Also, I changed the messages to be lower case and removed a full stop.

Diff Detail

Event Timeline

grimar created this revision.Sep 4 2019, 8:25 AM

Thanks for working on this. I think you need a few more tests to show that we no longer exit(1) where we previously were (i.e. that we can report multiple error messages). I see you have one for relocation references, but there are several other places that need testing too.

lib/ObjectYAML/ELFEmitter.cpp
280

at -> by

291

at -> by

422–423

Maybe you could have this function return void, and then have the caller check HasErrors?

459

Adding a test case to show that we can report multiple errors for this issue would be good.

595

This might be a bit of a bigger task than you had hoped for, and I'm okay if you want to push this suggestion to a later change, but it would be great if this could call some form of error handler passed in by the client of the library instead of directly calling WithColor::error(). That way the client can control how the errors are reported. See my lightning talk on error handling in libraries from the US conference a couple of years ago.

Regardless, setting HasError here seems fine, and obviously a default option of WithColor::error() is sensible.

612

Maybe this could be a continue instead of a return. That way all unknown sections will be picked up.

724–725

Maybe it's worth having something like this comment in toSymbolIndex?

grimar updated this revision to Diff 218905.Sep 5 2019, 6:33 AM
grimar marked 10 inline comments as done.
  • Addressed review comments, added test cases.
  • Added single quotes around symbol name in the error message: "unknown section referenced ... by YAML symbol 'X'". This matches another messages tool reports.
lib/ObjectYAML/ELFEmitter.cpp
422–423

Yes, this is probably better.

459

Done.

595

I'll be happy to investigate it deeper and suggest a follow-up patch to add a custom error handler to this lib.

724–725

I added a comment.

jhenderson added inline comments.Sep 6 2019, 1:41 AM
lib/ObjectYAML/ELFEmitter.cpp
373

I think we need a test case showing this can be called even after an earlier sh_link fails (e.g. two sections with bad sh_link values).

386

Arguably, the same goes for each of the changes here too - if the writeSectionContent can fail for a given section type, we should have a test showing that the loop continues.

716

It might be interesting to have a test case showing that a bad sh_info and an unknown symbol index are both reported for the same relocation section. Up to you.

774

Perhaps worth a test for a bad symbol and multiple bad section indexes in the same section.

947–948

Could this just be a continue and this function return void?

971

Could this be a continue and return void?

test/tools/yaml2obj/elf-comdat-broken-info.yaml
1–4

produce an SHT_GROUP

30

when multiple unknown symbols
by SHT_GROUP sections.

grimar updated this revision to Diff 219077.Sep 6 2019, 5:52 AM
grimar marked 15 inline comments as done.
  • Addressed review comments.
lib/ObjectYAML/ELFEmitter.cpp
373

Done in section-link.yaml.

386

I am not sure it worth testing so hard honestly.

There is nothing like

if (HasError)
  return;

around, so I think no reason to assume that loop can terminate.
It feels that testing a single writeSectionContent should be enough.

However, the current situation is following.
writeSectionContent can fail for:

  1. writeSectionContent(...const ELFYAML::DynamicSection &Section...)

It is tested in: dynamic-section-raw-content.yaml

  1. writeSectionContent(...const ELFYAML::RelocationSection &Section...)

I added a test to reloc-sec-info.yaml

  1. writeSectionContent(...const ELFYAML::Group &Section...)

It is tested in: elf-comdat-broken-info.yaml

716

I added a test to reloc-sec-info.yaml

774

I added elf-comdat-broken-members.yaml.

947–948

Yes, done.

971

Yes, done.

jhenderson accepted this revision.Sep 6 2019, 7:22 AM

LGTM, with comment fixes

test/tools/yaml2obj/elf-comdat-broken-members.yaml
1

refereces(typo) unknown -> references an unknown
members list -> member list

test/tools/yaml2obj/reloc-sec-info.yaml
27

when a relocation section
via its Info

test/tools/yaml2obj/section-link.yaml
27

when an unknown
via a Link

This revision is now accepted and ready to land.Sep 6 2019, 7:22 AM
MaskRay added inline comments.Sep 6 2019, 9:16 AM
lib/ObjectYAML/ELFEmitter.cpp
117

Instead of HasError, have you thought using an integer (int errorCount = 0;) ?

MaskRay added inline comments.Sep 6 2019, 9:18 AM
lib/ObjectYAML/ELFEmitter.cpp
117

Sorry, int ErrorCount = 0;

Is there a case that many errors may be accumulated? If yes, we may need an ErrorLimit. We certainly don't need ErrorLimit in this patch, but using ErrorCount may prevent a future rename HasError -> ErrorCount..

MaskRay added inline comments.Sep 6 2019, 9:25 AM
test/tools/yaml2obj/duplicate-symbol-names.test
44

CASE3-COUNT-2:

test/tools/yaml2obj/dynamic-section-raw-content.yaml
29

Can -o %t2 be deleted?

test/tools/yaml2obj/dynsymtab-implicit-sections-size-content.yaml
32

Can -o %t2 be deleted?

56

Can %t3 be deleted?

grimar marked 3 inline comments as done.Sep 6 2019, 11:09 AM
grimar added inline comments.
lib/ObjectYAML/ELFEmitter.cpp
117

I'd do this when we need it. It is not clear how much it is useful, since it is not the same case as we have in linker: I do not think there can be many errors in a hand written YAML document. And probably no need to limit them.
obj2yaml is assumed to produce only valid YAMLs too.

test/tools/yaml2obj/dynamic-section-raw-content.yaml
29

Yes, you're right here and below. I'll update the diff in my next week or tomorrow.

MaskRay accepted this revision.Sep 6 2019, 7:38 PM

LG once you remove the redundant -o %t2 in a few places.

lib/ObjectYAML/ELFEmitter.cpp
117

I was just checking whether we'll eventually do that. So using the final variable name can prevent a future rename. You make the call. I haven't put much thought on that, either.

grimar edited the summary of this revision. (Show Details)Sep 9 2019, 2:40 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 7 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 2:42 AM