This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Allow setting st_value explicitly for Symbol.
ClosedPublic

Authored by grimar on Apr 26 2019, 3:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Apr 26 2019, 3:49 AM

I have a slight concern here, in that the approach prevents us using a name that is actually a number, and that might be confusing. I think we might want to have a different field (e.g. "NameIndex" or similar) explicitly for this situation. One and only one of the two fields can then be specified.

What do you think?

grimar added a comment.EditedApr 29 2019, 8:56 AM

I have a slight concern here, in that the approach prevents us using a name that is actually a number, and that might be confusing. I think we might want to have a different field (e.g. "NameIndex" or similar) explicitly for this situation. One and only one of the two fields can then be specified.

What do you think?

I thought that it is impossible to have a symbol name that is a number. Because valid C name must begin with a letter of the alphabet or an underscore.
I.e. I am not sure we should care about this case. At least now. Should we?

I thought that it is impossible to have a symbol name that is a number. Because valid C name must begin with a letter of the alphabet or an underscore.
I.e. I am not sure we should care about this case. At least now. Should we?

Maybe not in C, but there's nothing in ELF that requires this, and I believe it might be possible to name them like that in assembly using double quotes.

grimar planned changes to this revision.Apr 30 2019, 3:42 AM

Ok. Planning changes. Will take some time since I will be off starting from tomorrow by EOW because of holidays here.

grimar updated this revision to Diff 198292.May 6 2019, 9:08 AM
  • Reimplemented as was suggested during the review.
grimar retitled this revision from [yaml2obj] - Treat integer symbol names as explicit st_name value. to [yaml2obj] - Allow setting st_value explicitly for Symbol..May 6 2019, 9:08 AM
jhenderson accepted this revision.May 7 2019, 4:50 AM

Some comment nits, otherwise LGTM.

test/tools/yaml2obj/symbol-name.yaml
1 ↗(On Diff #198292)

FWIW, I liked what you did in other tests with the double '#' symbols to indicate comments. I wouldn't object to you doing it here either.

tools/yaml2obj/yaml2elf.cpp
472 ↗(On Diff #198292)

name index containing offset -> NameIndex, which contains the name offset,
remove "in YAML", since theoretically, we could use this bit of code via other methods in the future.

473 ↗(On Diff #198292)

It is -> This is
a Name -> the specified Name

This revision is now accepted and ready to land.May 7 2019, 4:50 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 3 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2019, 5:11 AM