Page MenuHomePhabricator

[yaml2obj] - Add a technical suffix for each unnamed chunk.
ClosedPublic

Authored by grimar on Fri, May 15, 12:29 AM.

Details

Summary

This change does not affect the produced binary.

In this patch I assign a technical suffix to each section/fill
(i.e. chunk) name when it is empty. It allows to simplify the code
slightly and improve error messages reported.

In the code we have the section to index mapping, SN2I, which is
globally used. With this change we can use it to map "empty"
names to indexes now, what is helpful.

Diff Detail

Event Timeline

grimar created this revision.Fri, May 15, 12:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar edited the summary of this revision. (Show Details)
grimar edited the summary of this revision. (Show Details)
grimar added a comment.EditedFri, May 15, 12:38 AM

D79985 depends on this, but it can live without it. I am going to post another patch soon that is using SN2I and requires this change.

MaskRay added inline comments.Fri, May 15, 10:22 AM
llvm/test/tools/yaml2obj/ELF/section-link.yaml
32

Can we omit unnamed and use '' (index 2)?

grimar marked an inline comment as done.Sun, May 17, 3:31 AM
grimar added inline comments.
llvm/test/tools/yaml2obj/ELF/section-link.yaml
32

We can omit/add whatever we want into [ ... ] part.

This patch works because it automatically adds unique suffixes. What we use for
describing sections/symbols with the same name already:

- Name: '.foo [1]'
  Type: SHT_PROGBITS
- Name: '.foo [2]'
  Type: SHT_PROGBITS

I.e. we can add any [ any text here ] as suffix to a section name and the existent logic will
handle it properly. But we can't switch [] to () without doing additional changes.

It could be ' [index #2]', but not '' [index 2] nor '' (index 2).

It can be possible to change (add an additional name parsing of a section name) in
toSectionIndex to improve the message reported, but I am not sure it worth that additional complexity:

template <class ELFT>
unsigned ELFState<ELFT>::toSectionIndex(StringRef S, StringRef LocSec,
                                        StringRef LocSym) {
....
//// Add a code to parse LocSec/LocSym here.

  if (!LocSym.empty())
    reportError("unknown section referenced: '" + S + "' by YAML symbol '" +
                LocSym + "'");
  else
    reportError("unknown section referenced: '" + S + "' by YAML section '" +
                LocSec + "'");
  return 0;
}
MaskRay added inline comments.Sun, May 17, 11:09 PM
llvm/test/tools/yaml2obj/ELF/section-link.yaml
32

OK, this name should be fine. But why doesn't '' [index 2]" work? (I am actually ok with either one but #2 not working seems strange to me.)

grimar marked 2 inline comments as done.Mon, May 18, 12:48 AM
grimar added inline comments.
llvm/test/tools/yaml2obj/ELF/section-link.yaml
32

See.

  1. We have the following code:
StringRef llvm::ELFYAML::dropUniqueSuffix(StringRef S) {
  size_t SuffixPos = S.rfind(" [");
  if (SuffixPos == StringRef::npos)
    return S;
  return S.substr(0, SuffixPos);
}

I.e. we can safely add [ suffix and everything starting from it will be ignored.
The empty section name with a suffix looks like " [...", where ... is any data or nothing.
e.g " [ index 2 ]"/" [ foo"/" [" are valid empty section names.

  1. The code that reports an error is:
reportError("unknown section referenced: '" + S + "' by YAML section '" +
                LocSec + "'");

I.e. if the section name is X, it prints:

unknown section referenced: '.foo' by YAML section 'X'

How can it print '' [index 2]? For section name " [ index 2 ]" it would print:

unknown section referenced: '.foo' by YAML section ' [index 2]`
jhenderson added inline comments.Mon, May 18, 1:13 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
252

You refer to the name here as a prefix, but it's really a suffix. I think calling it a prefix is slightly confusing given it is supposed to be equivalent to the unique suffix used for identically-named sections.

Perhaps also worth mentioning that this should be identical style to the seciton name uniquing. In fact, if there's a way to enforce that identical property by code sharing, that would be even better.

252–254

Minor nit: comment flowing here seems a little weird (I'd expect "report" to be on the line before at least).

llvm/test/tools/yaml2obj/ELF/section-link.yaml
32

I can probably live with the bit in square brackets being inside the quotes, but is it possible to get rid of the extra space at least? That might require a little extra logic in dropUniqueSuffix to work, but I think it might be a nice property, and might even be more generally useful (allowing slight variations in whitespace in unique names without affecting behaviour).

grimar updated this revision to Diff 264584.Mon, May 18, 4:39 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.

Note: 2 test cases are failing with this change. See my related question inlined.

llvm/test/tools/yaml2obj/ELF/section-link.yaml
32

but is it possible to get rid of the extra space at least?

It is problematic because we have at least 2 tests that doesn't like it:
https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test#L111
https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/macro.yaml#L57

The best solution I see is to start using "<>" brackets instead of "[]" for unique suffixes.

I made a code change in this patch to show how it might work. So what about if I change []-><> first?

grimar retitled this revision from [yaml2obj] - Add a technical prefix for each unnamed chunk. to [yaml2obj] - Add a technical suffix for each unnamed chunk..Mon, May 18, 4:52 AM
grimar edited the summary of this revision. (Show Details)
grimar edited the summary of this revision. (Show Details)
grimar marked an inline comment as done.Mon, May 18, 7:30 AM
grimar added inline comments.
llvm/test/tools/yaml2obj/ELF/section-link.yaml
32

I've changed "[]" to "()" here: D80123

grimar updated this revision to Diff 264847.Tue, May 19, 4:05 AM
  • Updated.

Ready for review.

MaskRay added inline comments.Tue, May 19, 9:08 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
415

I think the code can be simplified if you change here to constexpr char SuffixStart = '('

420

s/a //

jhenderson accepted this revision.Wed, May 20, 1:26 AM

LGTM, once @MaskRay's comments have been addressed.

llvm/lib/ObjectYAML/ELFEmitter.cpp
420

We definitely want the first "a". I think the second one is acceptable either way, so I'd say either leave as-is or "Do not add a space when Name is empty."

This revision is now accepted and ready to land.Wed, May 20, 1:26 AM
grimar updated this revision to Diff 265192.Wed, May 20, 3:04 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/lib/ObjectYAML/ELFEmitter.cpp
415

Done.

@MaskRay, are you happy with the code change?

MaskRay accepted this revision.Fri, May 22, 12:05 PM
This revision was automatically updated to reflect the committed changes.