Page MenuHomePhabricator

[llvm-objcopy] - Implement replaceSectionReferences for GroupSection class.
ClosedPublic

Authored by grimar on Mar 21 2019, 6:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Mar 21 2019, 6:30 AM
grimar updated this revision to Diff 191673.Mar 21 2019, 6:35 AM
  • Minor cosmetic changes.

Looks good apart from a few comments.

test/tools/llvm-objcopy/ELF/compress-debug-sections-groups.test
5 ↗(On Diff #191671)

tool -> llvm-objcopy

(let's be explicit)

6 ↗(On Diff #191671)

section -> sections

8 ↗(On Diff #191671)

I'd tweak this and the below comments to the following, as it sounds more natural:

"Check compression of debug sections." (or decompression)

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

Could you elaborate please what you have in mind? We currently already update all the symbols in the symbol table.

Spec says (https://docs.oracle.com/cd/E19683-01/816-1386/chapter7-26/index.html):
"The name of a symbol from one of the containing object's symbol tables provides a signature for the section group."
In the test, I check that the group signature is still groupname both after compression and decompression,
i.e. I think I already check that nothing is broken here. Am I missing something?

grimar updated this revision to Diff 191682.Mar 21 2019, 7:02 AM
  • Addressed review comments.

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

Could you elaborate please what you have in mind? We currently already update all the symbols in the symbol table.

Spec says (https://docs.oracle.com/cd/E19683-01/816-1386/chapter7-26/index.html):
"The name of a symbol from one of the containing object's symbol tables provides a signature for the section group."
In the test, I check that the group signature is still groupname both after compression and decompression,
i.e. I think I already check that nothing is broken here. Am I missing something?

Yes, the symbols are correctly updated, and it's not possible to break the behaviour with the current switches available in llvm-objcopy, I think, but theoretically, there's nothing stopping us replacing the symbol table itself. The GroupSection class has a pointer to a SymbolTableSection, which will point at the wrong section if that symbol table ever gets replaced. In other words, I think you should probably add this to the replaceSectionReferences definition for strict correctness:

if (SectionBase *To = FromTo.lookup(SymTab))
  Symtab = To;

On the other hand, symbol table replacing will cause other issues, so if you don't want to do it, I guess that's okay for now?

Hmm, I suppose for strict correctness, you should also check if the associated symbol table needs updating too.

Could you elaborate please what you have in mind? We currently already update all the symbols in the symbol table.

Spec says (https://docs.oracle.com/cd/E19683-01/816-1386/chapter7-26/index.html):
"The name of a symbol from one of the containing object's symbol tables provides a signature for the section group."
In the test, I check that the group signature is still groupname both after compression and decompression,
i.e. I think I already check that nothing is broken here. Am I missing something?

Yes, the symbols are correctly updated, and it's not possible to break the behaviour with the current switches available in llvm-objcopy, I think, but theoretically, there's nothing stopping us replacing the symbol table itself. The GroupSection class has a pointer to a SymbolTableSection, which will point at the wrong section if that symbol table ever gets replaced. In other words, I think you should probably add this to the replaceSectionReferences definition for strict correctness:

if (SectionBase *To = FromTo.lookup(SymTab))
  Symtab = To;

On the other hand, symbol table replacing will cause other issues, so if you don't want to do it, I guess that's okay for now?

I see. Symbol table replacement would definitely require much more changes everywhere I think.
So, yes, I would prefer not to add a code for this a bit unrealistic (atm) case.

jhenderson accepted this revision.Mar 21 2019, 7:39 AM

Okay, LGTM. Maybe worth waiting a day to let others comment.

This revision is now accepted and ready to land.Mar 21 2019, 7:39 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 3:23 AM