Page MenuHomePhabricator

[yaml2obj][ELF] - Simplify the code that performs sections validation.
ClosedPublic

Authored by grimar on Oct 15 2020, 6:47 AM.

Details

Summary

This:

  1. Changes the return type of MappingTraits<T>>::validate to std::string

instead of StringRef. It allows to create more complex error messages.

  1. It introduces std::vector<std::pair<StringRef, bool>> getEntries():

a new virtual method of Section, which is the base class for all sections.
It returns names of special section specific keys (e.g. "Entries") and flags that says if them exist in a YAML.
The code in validate() uses this list of entries descriptions to generalize validation.
This approach was discussed in the D89039 thread.

Diff Detail

Event Timeline

grimar created this revision.Oct 15 2020, 6:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar requested review of this revision.Oct 15 2020, 6:47 AM

Implements one of approaches discussed in the D89039 thread, which allows to generalize sections validation.

Probably just copy some messages from https://reviews.llvm.org/D89039#inline-828862 to make the description self-contained

In short, this moves a bunch of checks from MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::validate to *::getEntries

llvm/lib/ObjectYAML/DWARFYAML.cpp
311

Seems inconsistent with {} you use below..

Implements one of approaches discussed in the D89039 thread, which allows to generalize sections validation.

Probably just copy some messages from https://reviews.llvm.org/D89039#inline-828862 to make the description self-contained

Will do.

In short, this moves a bunch of checks from MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::validate to *::getEntries

I'd say that checks are not moved. *::getEntries is a new virtual method of Section, which is the base class for all sections.
It just returns names of special section specific keys (e.g. "Entries") and flags that says if them exist in a YAML.
The code in validate() uses this list of entries descriptions to generalize validation. The validation and error reporting is still performed in validate().

grimar edited the summary of this revision. (Show Details)Oct 15 2020, 11:36 PM
MaskRay accepted this revision.Oct 19 2020, 8:58 PM

LGTM.

This revision is now accepted and ready to land.Oct 19 2020, 8:58 PM

Just seen you committed it whilst I was mid review. My comments still stand :-)

llvm/include/llvm/ObjectYAML/ELFYAML.h
196–197
198

Maybe rather than a vector of pairs, a StringMap<bool> might be more straightforward. Not sure if order is an issue though.

245

Seems more easily understandable to me. Same throughout.

llvm/lib/ObjectYAML/DWARFYAML.cpp
311

FWIW, I'd prefer "" throuhgout since it's more readable (you don't have to refer to the return type to figure out that it's a string-like thing), but don't feel strongly about it.

llvm/lib/ObjectYAML/ELFYAML.cpp
1440

Traditional LLVM style says to precalculate the size, rather than to do it on each iteration of the loop.

Just seen you committed it whilst I was mid review. My comments still stand :-)

Sorry! I'll address them soon.

grimar marked 3 inline comments as done.Oct 20 2020, 2:52 AM

Post commit comments are addressed in rG3be2b0d1a1e0b94e8586f695e8174f139f7a84d8

llvm/include/llvm/ObjectYAML/ELFYAML.h
198

I think the order might be an issue as we build and then check errors we report. I am not sure anout using map,
as this method is only useful for validate() where we want to iterate over all entries and never to lookup a particular name.
If you really want, I think I can replace it with MapVector<StrinfRef, bool> though.

llvm/lib/ObjectYAML/DWARFYAML.cpp
311

I've switched to using "" in other places too for consistency. Perhaps, for string types it is a more natural empty value than {}.

@grimar This appears to be affecting clang-tidy unit tests - please can you take a look?

http://lab.llvm.org:8011/#/builders/109/builds/770
http://lab.llvm.org:8011/#/builders/109/builds/783

grimar marked an inline comment as done.Oct 20 2020, 5:15 AM

@grimar This appears to be affecting clang-tidy unit tests - please can you take a look?

http://lab.llvm.org:8011/#/builders/109/builds/770
http://lab.llvm.org:8011/#/builders/109/builds/783

Sorry. Reverting right now.