[PATCH] D59638: [llvm-objcopy] - Implement replaceSectionReferences for GroupSection class.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D59638#1437838 <https://reviews.llvm.org/D59638#1437838>, @grimar wrote:

> In D59638#1437787 <https://reviews.llvm.org/D59638#1437787>, @jhenderson wrote:
>
> > 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59638/new/

https://reviews.llvm.org/D59638