This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Change how we handle implicit sections.
ClosedPublic

Authored by grimar on Jul 19 2019, 8:40 AM.

Details

Summary

Instead of having the special list of implicit sections,
that are mixed with the sections read from YAML on late
stages, I just create the placeholders and add them to
the main sections list early.

That allows to significantly simplify the code.

Diff Detail

Event Timeline

grimar created this revision.Jul 19 2019, 8:40 AM
grimar added a comment.EditedJul 19 2019, 8:49 AM

As an alternative if we do not want to make document non-const, I can introduce a new list of Section *
and populate it using Doc.Sections and new sections. yaml2elf will refer to this new list instead of Doc.Sections everywhere.
I did not try, but this also should work.

(i.e. it will be almost the same patch like now, just needs an additional list(s). I am not sure how much important to keep the const here, this approach is a bit simpler)

MaskRay added inline comments.Jul 21 2019, 11:21 PM
include/llvm/ObjectYAML/ELFYAML.h
153

Prefer in-class default member initializer bool IsImplicit = false;

But I think changing the signature to Section(SectionKind Kind, bool IsImplicit) may look better (you'll have to fix 9 ctor calls in this file).

tools/yaml2obj/yaml2elf.cpp
205
Doc.Sections.emplace_back(ELFYAML::Section::SectionKind::RawContent, true)
Doc.Sections.back()->Name = SecName; // After llvm migrates to C++17, we can leverage the reference return type of emplace_back.
```;
grimar updated this revision to Diff 211025.Jul 22 2019, 12:49 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
tools/yaml2obj/yaml2elf.cpp
205

Doc.Sections.emplace_back(ELFYAML::Section::SectionKind::RawContent, true)

I can't do this. Doc.Sections is a vector of unique_ptr<Section> And compiler reasonably barks:

1>C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\xmemory0(881): error C2664: 'std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)': cannot convert argument 1 from '_Ty' to 'llvm::ELFYAML::Section *'
1> with
1> [
1> _Ty=llvm::ELFYAML::Section
1> ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\xmemory0(881): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast
1>C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\vector(902): note: see reference to function template instantiation 'void std::_Default_allocator_traits<_Alloc>::construct<_Ty,llvm::ELFYAML::Section::SectionKind,bool>(_Alloc &,_Objty *const ,llvm::ELFYAML::Section::SectionKind &&,bool &&)' being compiled
1> with
1> [
1> _Alloc=std::allocator<std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<llvm::ELFYAML::Section>>>,
1> _Ty=std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<llvm::ELFYAML::Section>>,
1> _Objty=std::unique_ptr<llvm::ELFYAML::Section,std::default_delete<llvm::ELFYAML::Section>>
1> ]

grimar marked an inline comment as not done.Jul 22 2019, 12:50 AM
jhenderson added inline comments.Jul 22 2019, 1:37 AM
include/llvm/ObjectYAML/ELFYAML.h
155

Perhaps worth considering making IsImplicit a default argument?

tools/yaml2obj/yaml2elf.cpp
195

Just a thought, not really related to this change, but shouldn't the presence of .symtab and .strtab be dependent on there being any symbols defined (just like .dynsym/.dynstr)?

grimar marked an inline comment as done.Jul 22 2019, 1:49 AM
grimar added inline comments.
tools/yaml2obj/yaml2elf.cpp
195

Perhaps having .strtab (even an empty one, i.e. only with a zero entry) is a bit more common?

I am not sure actually, but if you take an empty test.s and compile it with llvm-mc, you'll get a file which has:

Symbol table '.symtab' contains 1 entry:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
jhenderson added inline comments.Jul 22 2019, 2:31 AM
tools/yaml2obj/yaml2elf.cpp
195

I guess it doesn't really matter either way.

grimar updated this revision to Diff 211044.Jul 22 2019, 3:45 AM
grimar marked an inline comment as done.
  • Addressed review comments.
MaskRay accepted this revision.Jul 22 2019, 4:04 AM
This revision is now accepted and ready to land.Jul 22 2019, 4:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 5:03 AM