Page MenuHomePhabricator

[yaml2obj][obj2yaml] - Do not create a symbol table by default.
ClosedPublic

Authored by grimar on Oct 16 2019, 8:00 AM.

Details

Summary

This patch tries to resolve problems faced in D68943
and uses some of the code written by Konrad Wilhelm Kleine
in that patch.

Previously, yaml2obj tool always created a .symtab section.
This patch changes that. With it we only create it when
have a "Symbols:" tag in the YAML document or when
we need to create it because it is used by another section(s).

obj2yaml follows the new behavior and does not print "Symbols:"
anymore when there is no symbol table.

It touches and changes many test cases and also reveals 2 issues
(one of them is a crash) in llvm-objcopy tool. I added a TODO mark
with the descriptions to their test cases.

Diff Detail

Event Timeline

grimar created this revision.Oct 16 2019, 8:00 AM
grimar edited the summary of this revision. (Show Details)Oct 16 2019, 8:05 AM
MaskRay added inline comments.Oct 16 2019, 9:20 AM
lib/ObjectYAML/ELFEmitter.cpp
208 ↗(On Diff #225223)

if (Doc.Symbols) continue; can be factored out.

test/tools/yaml2obj/symtab-implicit-sections-flags.yaml
80 ↗(On Diff #225223)

In other places, you seem to use Symbols: []

kwk added a comment.EditedOct 16 2019, 12:38 PM

@grimar I don't understand why you created this patch, when I'm working on it already.

lib/ObjectYAML/ELFEmitter.cpp
208 ↗(On Diff #225223)

I agree with this optimization.

But I also wonder why you say that some section link to .symtab by default. So far I've only seen explicit references to .symtab in the Link: .symtab entry of a section in a test file. So why not change the test files to point to an existing section (you changed most of them anyways)? Or will a an empty Link: entry automatically point to .symtab?

232 ↗(On Diff #225223)

Change to ImplicitSections.insert(ImplicitSections.end(), {".strtab", ".shstrtab"});

test/Object/invalid.test
295 ↗(On Diff #225223)

Isn't Symbols: enough?

test/tools/llvm-objcopy/ELF/segment-test-remove-section.test
45 ↗(On Diff #225223)

I suggest to use TODO(username): .

test/tools/yaml2obj/implicit-sections-types.test
28 ↗(On Diff #225223)

See, apparently it is enough ;)

test/tools/yaml2obj/implicit-sections.test
88 ↗(On Diff #225223)

Great, that you've put this all into one file.

rupprecht added inline comments.Oct 16 2019, 2:35 PM
lib/ObjectYAML/ELFEmitter.cpp
209–210 ↗(On Diff #225223)

I think SHT_HASH, SHT_GROUP, and SHT_SYMTAB_SHNDX might also belong here, per: http://www.sco.com/developers/gabi/latest/ch4.sheader.html#sh_link

At that point, it might look cleaner to refactor out the list as a separate variable that is checked for list/set containment

214–218 ↗(On Diff #225223)

This case is interesting, partly because IIRC we raise an error if someone specifies a link to a section that does not exist, so special casing this seems unexpected. However, I imagine many tests would need to be fixed and it's maybe not worth it -- have you checked how many tests would fail if you didn't special case this?

231 ↗(On Diff #225223)

Is .strtab also something we can drop if .symtab doesn't exist?

In D69041#1711707, @kwk wrote:

@grimar I don't understand why you created this patch, when I'm working on it already.

I only wanted to help, because supposed that the correct direction is not clear for you (since your version did not fix any of existent test cases that were failing,
obj2yaml tool did not compile and there was at least one obliously wrong place in 'initSymtabSectionHeader' that broke the normal logic).
And it was also not fully clear to me what is exactly needed until I run check-llvm with the first more or less stable version of the patch at least.

Now when the whole picture is revealed, feel free to commandeer this revision or use it as a reference for your patch if you would like to continue it.
Or let me continue. Please let me know about your decision, I'll put this on hold until that.

kwk added a comment.Oct 17 2019, 2:22 AM
In D69041#1711707, @kwk wrote:

@grimar I don't understand why you created this patch, when I'm working on it already.

I only wanted to help, because supposed that the correct direction is not clear for you (since your version did not fix any of existent test cases that were failing,
obj2yaml tool did not compile and there was at least one obliously wrong place in 'initSymtabSectionHeader' that broke the normal logic).
And it was also not fully clear to me what is exactly needed until I run check-llvm with the first more or less stable version of the patch at least.

Now when the whole picture is revealed, feel free to commandeer this revision or use it as a reference for your patch if you would like to continue it.
Or let me continue. Please let me know about your decision, I'll put this on hold until that.

@grimar I'm really sorry that I wrote this yesterday. I'm working crazy hours lately and wasn't in a bad mood. I understand why you wrote the patch now. But since it wasn't something urgent to do and only motivated because I brought it up, I didn't fully understood how little time I was given to react to your comments in my own patch. I will discontinue it as you obviously have more experience in the yaml2obj and obj2yaml code. What's bugging me is just that I wanted to contribute and didn't have enough time to properly do so, that's all.

In D69041#1712474, @kwk wrote:
In D69041#1711707, @kwk wrote:

@grimar I don't understand why you created this patch, when I'm working on it already.

I only wanted to help, because supposed that the correct direction is not clear for you (since your version did not fix any of existent test cases that were failing,
obj2yaml tool did not compile and there was at least one obliously wrong place in 'initSymtabSectionHeader' that broke the normal logic).
And it was also not fully clear to me what is exactly needed until I run check-llvm with the first more or less stable version of the patch at least.

Now when the whole picture is revealed, feel free to commandeer this revision or use it as a reference for your patch if you would like to continue it.
Or let me continue. Please let me know about your decision, I'll put this on hold until that.

@grimar I'm really sorry that I wrote this yesterday. I'm working crazy hours lately and wasn't in a bad mood. I understand why you wrote the patch now. But since it wasn't something urgent to do and only motivated because I brought it up, I didn't fully understood how little time I was given to react to your comments in my own patch. I will discontinue it as you obviously have more experience in the yaml2obj and obj2yaml code. What's bugging me is just that I wanted to contribute and didn't have enough time to properly do so, that's all.

No problems. Hope we will land it soon :)

In D69041#1711707, @kwk wrote:

@grimar I don't understand why you created this patch, when I'm working on it already.

@kwk Sorry if you feel your work is commandeered. On D68943, it was me who said

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).

I skimmed through your yaml2obj changes in D68943 and I believed a complete fix needs more work than your change. check-llvm-tools check-llvm-object should expose more tests that you missed. George has done tons of work in yaml2obj/obj2yaml in the recent months and he probably knows the code better than anyone else. If you read this patch more carefully, you should notice that your version differs a lot from his version.

Apologies if you feel sad, but as I said you should still get credits for your lldb changes. Why do you abandon D68943? You can still keep the lldb part.

kwk added a comment.Oct 17 2019, 2:49 AM
In D69041#1711707, @kwk wrote:

@grimar I don't understand why you created this patch, when I'm working on it already.

@kwk Sorry if you feel your work is commandeered. On D68943, it was me who said

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).

I skimmed through your yaml2obj changes in D68943 and I believed a complete fix needs more work than your change. check-llvm-tools check-llvm-object should expose more tests that you missed.

I know (D68943#1708113) that I did miss tests and simply haven't updated my diff. I was still fixing some tests locally (D68943#1709330). I didn't ask nor expected somebody to run tests.

George has done tons of work in yaml2obj/obj2yaml in the recent months and he probably knows the code better than anyone else. If you read this patch more carefully, you should notice that your version differs a lot from his version.

The basic idea is the same only that he cover's Link: entries as well and has fixed more tests (which I also had locally).

Apologies if you feel sad, but as I said you should still get credits for your lldb changes. Why do you abandon D68943? You can still keep the lldb part.

I will add the LLDB part (which was only about removing .symtab manually) once Geroge's test has landed. Let's keep my feelings out of this for a bit ;)

MaskRay added inline comments.Oct 17 2019, 3:46 AM
lib/ObjectYAML/ELFEmitter.cpp
209–210 ↗(On Diff #225223)

sh_link(.hash) = .dynsym (not .symtab)

For SHT_GROUP and SHT_SYMTAB_SHNDX, we don't have implicit sh_link = .symtab rule.

It does raise the question: shall we keep the number of implicit .symtab rules low? Are we happy to keep the implicit rule for SHT_REL and SHT_RELA?

212 ↗(On Diff #225223)

Doc.Symbols.emplace()

Same below.

test/tools/yaml2obj/implicit-sections.test
88 ↗(On Diff #225223)

Typo: spicified

I think the YAML term is mapping key, or key, not tag. Tag refers to a different thing (https://yaml.org/spec/1.2/spec.html#id2764295).

91 ↗(On Diff #225223)

This can be FileCheck /dev/null --implicit-check-not=.symtab if you feel it is clearer.

102 ↗(On Diff #225223)

Typo: spicified -> specified

grimar updated this revision to Diff 225423.Oct 17 2019, 6:56 AM
grimar marked 22 inline comments as done.
  • Addressed review comments.
grimar added inline comments.Oct 17 2019, 6:56 AM
lib/ObjectYAML/ELFEmitter.cpp
208 ↗(On Diff #225223)

But I also wonder why you say that some section link to .symtab by default. So far I've only seen explicit references to .symtab in the Link: .symtab entry of a section in a test file.
Or will a an empty Link: entry automatically point to .symtab?

For SHT_REL[A] we set it to .symtab when there is no Link at all (and .strtab is mandatory to have in this case atm):
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFEmitter.cpp#L722

The same we do for SHT_LLVM_ADDRSIG (though in this case there will be no error when .strtab is missing).
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFEmitter.cpp#L1007

209–210 ↗(On Diff #225223)

At that point, it might look cleaner to refactor out the list as a separate variable that is checked for list/set containment

It does raise the question: shall we keep the number of implicit .symtab rules low? Are we happy to keep the implicit rule for SHT_REL and SHT_RELA?

Interesting question. So what about we do a change to yaml2obj to remove the additional logic added here?
I.e.:

  1. <I've done this> Remove the logic for SHT_LLVM_ADDRSIG from here. SHT_LLVM_ADDRSIG just sets the Link to 0 when there is no .symtab already. So it was a quick change on a test suite side.
  2. Keep the logic for SHT_REL and SHT_RELA here in this patch for now.
  3. Remove it and change our logic in a follow up, i.e. change it to stop producing errors for relocation sections when there is no .symtab section. Rel[A] Sections will start to get 0 as a Link value. Just like we do for SHT_LLVM_ADDRSIG currently.
214–218 ↗(On Diff #225223)

Good point. There were about 20 of tests, but I've fixed them instead. It reduces the number of rules, what makes sence too I think.

231 ↗(On Diff #225223)

Yes, seems we can do this. I tried and observed a few test case failtures (because of section indices changes mostly it seems),
and a crash (seems we need to handle a case when there is no "Symbols:" key, but there is a SHT_SYMTAB described.
It wants to have a .strtab currently).
Shouldn't be too hard to fix, but if you do not mind, I'd also leave it for a one more independent follow-up.

test/Object/invalid.test
295 ↗(On Diff #225223)

Yes, but [ ] shows the intention a bit better I think.

test/tools/yaml2obj/implicit-sections.test
88 ↗(On Diff #225223)

I think the YAML term is mapping key, or key, not tag. Tag refers to a different thing (https://yaml.org/spec/1.2/spec.html#id2764295).

Seems might want to fix another comments we already have :)

MaskRay added inline comments.Oct 17 2019, 8:29 AM
lib/ObjectYAML/ELFEmitter.cpp
209–210 ↗(On Diff #225223)

All points look good. We can do 1 and 2 in this patch. We can defer 3 to a future change.

tools/obj2yaml/elf2yaml.cpp
205 ↗(On Diff #225423)

Y->Symbols.emplace();

Generally LGTM, although I think Ray is a much better person to approve this.

lib/ObjectYAML/ELFEmitter.cpp
209–210 ↗(On Diff #225223)

For SHT_GROUP and SHT_SYMTAB_SHNDX, we don't have implicit sh_link = .symtab rule.

I'm not sure I understand this bit. Why not? According to my link, sh_link for those is "The section header index of the associated symbol table" -- is there a reference saying otherwise? (Not saying you're wrong, just looking for clarification).

231 ↗(On Diff #225223)

SGTM, no rush

test/tools/llvm-objcopy/ELF/add-symbol-new-symbol-visibility.test
18–20 ↗(On Diff #225223)

Hopefully you can revert these files after rL375105

grimar marked 2 inline comments as done.Oct 17 2019, 11:37 AM
grimar added inline comments.
lib/ObjectYAML/ELFEmitter.cpp
209–210 ↗(On Diff #225223)

I'm not sure I understand this bit. Why not? According to my link, ...

Yeah, the link is correct. But we just do not implicity set sh_link to anything in yaml2obj for SHT_GROUP and SHT_SYMTAB_SHNDX yet.
(In compare with SHT_LLVM_ADDRSIG , for example, where we do). We might want to implement it, it is just a missing I think.

test/tools/llvm-objcopy/ELF/add-symbol-new-symbol-visibility.test
18–20 ↗(On Diff #225223)

Yes. The latest diff does not have these lines anymore.

grimar updated this revision to Diff 225489.Oct 17 2019, 11:50 AM
grimar marked an inline comment as done.
  • Addressed review comment (push_back -> emplace).
MaskRay accepted this revision.Oct 18 2019, 7:18 PM
This revision is now accepted and ready to land.Oct 18 2019, 7:18 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2019, 7:46 AM