Page MenuHomePhabricator

[llvm][yaml2obj] no more implicit ELF .symtab section
AbandonedPublic

Authored by kwk on Oct 14 2019, 5:41 AM.

Details

Summary

Before yaml2obj <FILE> would always add an implicit .symtab section
even if there were no symbols defined in the YAML <FILE>. Now, only
when there's a Symbols entry, we will generate a .symtab section.

Old minidebuginfo tests that manually removed the .symtab section
have been adjusted because it is no longer needed.

Event Timeline

kwk created this revision.Oct 14 2019, 5:41 AM
Herald added a reviewer: alexshap. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
kwk updated this revision to Diff 224838.Oct 14 2019, 5:46 AM
  • Silence FileCheck in test
Harbormaster completed remote builds in B39512: Diff 224838.
kwk updated this revision to Diff 224839.Oct 14 2019, 5:49 AM
  • restore formatting

+llvm yaml2obj folks.

Thank you for creating this diff. Please see comments inline.

lldb/test/Shell/ObjectFile/ELF/no-symtab-generated.yaml
1 ↗(On Diff #224838)

This test (and the next one) should live in the llvm subtree, as they're really testing yaml2obj, not lldb (instead of lldb, you can use something like llvm-objdump/readobj to inspect the generated files).

llvm/include/llvm/ObjectYAML/ELFYAML.h
379–380

This creates an inconsistency between how .symtab and .dynsym sections are handled, which is hard to justify. I actually prefer the Optional<> version, because that enables one to say "the symtab section is there, but it is _empty_", something which is impossible with the way .dynsym emission works (the section gets surpressed if it is empty), but the question is, shouldn't then the .dynsym emission work the same way ? Have you looked at by any chance how hard it would be to change .dynsym to follow the same pattern?

(In any case, I think this is up to yaml2obj maintainers to decide how to handle this. I'll add some to this review.)

kwk planned changes to this revision.Oct 14 2019, 5:59 AM

Move tests over to llvm/llvm/test/ObjectYAML/ELF/.

kwk marked an inline comment as done.Oct 14 2019, 6:01 AM
kwk added inline comments.
llvm/include/llvm/ObjectYAML/ELFYAML.h
379–380

@labath I haven't looked but will investigate how hard it is to declare Optional<std::vector<Symbol>> DynamicSymbols;. That's what I understood at least.

kwk updated this revision to Diff 224852.Oct 14 2019, 6:44 AM
  • moved test files over to llvm subtree
kwk marked an inline comment as done.Oct 14 2019, 6:45 AM

Moved yaml2obj tests under llvm subtree.

This is not correct. A lot of yaml2obj tests fail with:

yaml2obj: error: unknown section referenced: '.symtab' by YAML section '.rela.text'

The problem is that .symtab can be the sh_link field of relocation sections, .llvm_addrsig, and some sections that may be added in the future.

Making .symtab conditionally define is difficult. I'll take a closer look tomorrow.

kwk added a comment.Oct 14 2019, 8:35 AM

This is not correct. A lot of yaml2obj tests fail with:

yaml2obj: error: unknown section referenced: '.symtab' by YAML section '.rela.text'

@MaskRay I'm so sorry. You're absolutely right. I was working only in LLDB for the last months and used check-lldb only. That's probably because I missed those errors.

The problem is that .symtab can be the sh_link field of relocation sections, .llvm_addrsig, and some sections that may be added in the future.

Making .symtab conditionally define is difficult. I'll take a closer look tomorrow.

Thank you. Meanwhile I will run more tests to see what exactly fails.

I have not looked what is needed to fix yaml2obj testcases yet, but below there are
few first comments.

Also, 2 LLVM side tests added should live in "llvm/test/tools/yaml2obj" I think (not in llvm/test/ObjectYAML/ELF/),
because this is actually the place where we test "yaml2obj" usually.

llvm/lib/ObjectYAML/ELFEmitter.cpp
513

It doesn't look correct. We should never use dynamic symbols instead of static symbols I believe.

llvm/test/ObjectYAML/ELF/no-symtab-generated.yaml
14 ↗(On Diff #224852)

You do not need any sections.

18 ↗(On Diff #224852)

We often use the following order:

  1. Run lines
  2. Check lines
  3. YAML

I.e. the order you have used in symtab-generated.yaml. I'd suggest to be consistent and stick to it.

Also, probably would be better to have a single test case file, i.e. to merge them. You can see other our tests which do that.

llvm/test/ObjectYAML/ELF/symtab-generated.yaml
1 ↗(On Diff #224852)

We usually use ## for comments in yaml2obj tests.

31 ↗(On Diff #224852)

You do not need Entry.

33 ↗(On Diff #224852)

You do not need any sections it seems.

41 ↗(On Diff #224852)

Having only a "Name" should be enough.

kwk updated this revision to Diff 224968.Oct 15 2019, 1:47 AM
kwk marked 6 inline comments as done.
  • Remove unnecessary section
  • Have consitent ordering of run, check and YAML lines
  • use ## for comments in YAML
  • Remove not needed Entry and Sections
  • Only have Name for a symbol
  • Moved test files from llvm/test/ObjectYAML/ELF/ to llvm/test/tools/yaml2obj/
  • Fixup for test files
kwk added a comment.Oct 15 2019, 1:47 AM

@grimar I've tackled the most obvious of your comments. I will now see what fails in llvm/test/tools/yaml2obj.

kwk added a comment.Oct 15 2019, 4:55 AM

I noticed that the order of sections does matter to some tests. Before the .symtab section was always the first one and now it is the last of the implicit sections being created. Some Link: X checks break because X points to a different section. In order to compensate for this I will make .symtab the first of the implicit sections again. In my local testing this already helps a bit. In cases where we refer to a section by number and expect .symtab to be the section being referenced, I have two options: 1st) create a symbol to force .symtab to be implicitly created or 2nd) change the name of the expected section. Just tell me what you want.

grimar added a comment.EditedOct 15 2019, 5:13 AM
In D68943#1709330, @kwk wrote:

I noticed that the order of sections does matter to some tests. Before the .symtab section was always the first one and now it is the last of the implicit sections being created. Some Link: X checks break because X points to a different section. In order to compensate for this I will make .symtab the first of the implicit sections again.

Please upload the version with this change and I'll try to debug to take a look what happens.

Ok, I was able to debug it finally.

I think we should add a .symtab in a few more cases implicitly.
For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
i.e. it refers to .symtab by default and we should provide it.
There are other cases, like when Link just explicitly refers to .symtab.

Here is a patch based on this one.
It is unpolished, but shows the idea. With it all yaml2obj tests pass.

Ok, I was able to debug it finally.

I think we should add a .symtab in a few more cases implicitly.
For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
i.e. it refers to .symtab by default and we should provide it.
There are other cases, like when Link just explicitly refers to .symtab.

Here is a patch based on this one.
It is unpolished, but shows the idea. With it all yaml2obj tests pass.

Nice! Can you post a differential for the yaml2obj change? I believe the differential can focus on lldb and remove yaml2obj changes (kwk will still get credited for the test cleanups).

Ok, I was able to debug it finally.

I think we should add a .symtab in a few more cases implicitly.
For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
i.e. it refers to .symtab by default and we should provide it.
There are other cases, like when Link just explicitly refers to .symtab.

Here is a patch based on this one.
It is unpolished, but shows the idea. With it all yaml2obj tests pass.

Nice! Can you post a differential for the yaml2obj change? I believe the differential can focus on lldb and remove yaml2obj changes (kwk will still get credited for the test cleanups).

Yes. It needs polishing + fixes for obj2yaml (I do not think it compiled for me at all). Also probably makes sence to add tests for obj2yaml too, I need to take a look what it does
when the object doesn't have symbol table with such change. I'll try to suggest a patch today.

Thank you guys for jumping onto this. This will be very useful in lldb.

kwk abandoned this revision.EditedOct 17 2019, 2:23 AM

Abandoning for D69041