Page MenuHomePhabricator

[ObjectYAML] [COFF] Support multiple symbols with the same name
ClosedPublic

Authored by mstorsjo on Jan 3 2019, 3:07 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Jan 3 2019, 3:07 PM

I think a composite test (or tests) would makes sense, but check the intermediate output as well as the final output.

include/llvm/ObjectYAML/COFFYAML.h
62 ↗(On Diff #180151)

SymbolTableIndex -> SymbolIndex

"SymbolTableIndex" to me implies the section index of the symbol table.

tools/obj2yaml/coff2yaml.cpp
146 ↗(On Diff #180151)

SymbolOccurrances -> SymbolOccurrences

tools/yaml2obj/yaml2coff.cpp
531–533 ↗(On Diff #180151)

It should probably be an error somewhere if both fields are set (possibly unless they indicate the same symbol). I'd also write this as a ternary, with the symbol index being the first case, to avoid the lookup if not necessary, unless the intention is to insert a default constructed symbol in the map when no symbol exists?

mstorsjo marked 3 inline comments as done.Jan 4 2019, 5:10 AM
mstorsjo added inline comments.
include/llvm/ObjectYAML/COFFYAML.h
62 ↗(On Diff #180151)

Well the field in the actual relocation also is called SymbolTableIndex, so I chose that as it's an exact match of what's stored on disk. In coff, the symbol table isn't stored in a section, so there shouldn't be any confusion with that (in a pure coff environment).

tools/obj2yaml/coff2yaml.cpp
146 ↗(On Diff #180151)

Ok, will fix.

Later I though of using a StringMap<bool> SymbolUnique instead, to conserve space a little.

tools/yaml2obj/yaml2coff.cpp
531–533 ↗(On Diff #180151)

Ok, that sounds sensible. It's not intended to insert a dummy element in the map (although I doubt that it'd be harmful either).

I'll see if it making it an error here fits in.

jhenderson added inline comments.Jan 4 2019, 5:46 AM
include/llvm/ObjectYAML/COFFYAML.h
62 ↗(On Diff #180151)

Okay, fair enough. Keep it as is.

tools/obj2yaml/coff2yaml.cpp
146 ↗(On Diff #180151)

Yes, that would make sense.

mstorsjo updated this revision to Diff 180311.Jan 4 2019, 1:24 PM
mstorsjo edited the summary of this revision. (Show Details)

Added a testcase, applied @jhenderson's suggestions (erroring out if both SymbolName and SymbolTableIndex are present, avoiding touching SymbolTableIndexMap if direct index is set, changed the (misspelled) SymbolOccurrances map into a SymbolUnique map).

ruiu added inline comments.Jan 4 2019, 1:36 PM
tools/obj2yaml/coff2yaml.cpp
208 ↗(On Diff #180311)

Doesn't this insert a new key if *SymbolNameOrErr does not exist in the map? I don't think that affects correctness of the program, but that's probably a waste of time and memory. I'd use count instead.

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.
tools/obj2yaml/coff2yaml.cpp
208 ↗(On Diff #180311)

Yes, but every symbol will exist in the map already. We're not interested in whether the key exists in the map or not (it will always exist), but whether the map entry actually is set to true or false.

ruiu added inline comments.Jan 4 2019, 1:51 PM
tools/obj2yaml/coff2yaml.cpp
208 ↗(On Diff #180311)

Ah OK, then this is fine. I'd still use count to not surprise careless readers like me though. :)

mstorsjo marked an inline comment as done.Jan 4 2019, 1:55 PM
mstorsjo added inline comments.
tools/obj2yaml/coff2yaml.cpp
208 ↗(On Diff #180311)

Why count? Wouldn't that just return 1 saying that the symbol indeed is present in the map? We're not interested in whether the key is present or not (it always is), we're interested in the value of the map element, and count won't help with that.

mstorsjo marked an inline comment as done.Jan 4 2019, 2:00 PM
mstorsjo added inline comments.
tools/obj2yaml/coff2yaml.cpp
208 ↗(On Diff #180311)

You're imagining a StringSet that only contains the symbols we know are unique. It's not that (building such a data structure would be tricky) but a StringMap<bool> which could be called StringIsUnique, which stores all symbols and whether they are unique or not, as a bool.

ruiu added inline comments.Jan 4 2019, 2:02 PM
tools/obj2yaml/coff2yaml.cpp
208 ↗(On Diff #180311)

Sorry, what I wanted to say is lookup.

mstorsjo marked 4 inline comments as done.Jan 4 2019, 2:21 PM
mstorsjo added inline comments.
tools/obj2yaml/coff2yaml.cpp
208 ↗(On Diff #180311)

Oh, ok - I can change it to that.

mstorsjo updated this revision to Diff 180321.Jan 4 2019, 2:22 PM
mstorsjo marked an inline comment as done.

Changed [] into .lookup(), as requested by @ruiu.

alexshap added inline comments.Jan 4 2019, 2:47 PM
include/llvm/ObjectYAML/COFFYAML.h
62 ↗(On Diff #180151)

I'd probably add a comment somewhere explaining that this "duplication" (the presence of both Index and Name) are for convenience (that i.e. in YAML one can specify either the name or the index) + handling of non-unique names. P.S. if i understood the rationale for this change correctly, of course

mstorsjo marked 2 inline comments as done.Jan 4 2019, 2:54 PM
mstorsjo added inline comments.
include/llvm/ObjectYAML/COFFYAML.h
62 ↗(On Diff #180151)

Sure, I can add a comment.

mstorsjo updated this revision to Diff 180329.Jan 4 2019, 2:54 PM
mstorsjo marked an inline comment as done.

Added a comment to the COFFYAML.h header.

alexshap accepted this revision.Jan 4 2019, 3:39 PM

LGTM with one minor nit

tools/obj2yaml/coff2yaml.cpp
151 ↗(On Diff #180329)

side note: double lookup (1st - find, second - operator []) can be avoided here if we switch to using the method "insert" instead (it returns the corresponding iterator).

This revision is now accepted and ready to land.Jan 4 2019, 3:39 PM
mstorsjo marked 2 inline comments as done.Jan 5 2019, 2:32 AM
mstorsjo added inline comments.
tools/obj2yaml/coff2yaml.cpp
151 ↗(On Diff #180329)

Oh, nice, will do.

mstorsjo updated this revision to Diff 180363.Jan 5 2019, 2:35 AM
mstorsjo marked an inline comment as done.

Using the insert method, to avoid double lookup. Will push next week after giving @jhenderson a chance to comment, unless there's more things to change.

jhenderson accepted this revision.Jan 7 2019, 2:46 AM

I'm giving a conditional LGTM, assuming that the mentioned YAML blob is as small as reasonably possible to test the relocations. Remember, we don't need to have a fully usable object file, we just need one that has the relevant symbols and relocations.

include/llvm/ObjectYAML/COFFYAML.h
61 ↗(On Diff #180363)

To clarify that this comment refers to the two following variables, please put a new line before it.

test/tools/yaml2obj/coff-symbol-index.yaml
45 ↗(On Diff #180363)

Is this YAML blob as minimal as it can get whilst still testing the required properties? It's very verbose!

mstorsjo updated this revision to Diff 180455.Jan 7 2019, 4:04 AM

Added a newline before the comment in the header, minimized the yaml testcase.

jhenderson accepted this revision.Jan 7 2019, 4:38 AM

Okay, looking much better. I'm happy for this to go in, but if any of the other bits I highlighted can be removed, even better.

test/tools/yaml2obj/coff-symbol-index.yaml
34 ↗(On Diff #180455)

Are all these required? Same comment for the other sections.

59–61 ↗(On Diff #180455)

Are these fields required? Same question for further below.

78 ↗(On Diff #180455)

Is this necessary?

80 ↗(On Diff #180455)

Is this needed?

mstorsjo marked 8 inline comments as done.Jan 7 2019, 4:49 AM
mstorsjo added inline comments.
test/tools/yaml2obj/coff-symbol-index.yaml
34 ↗(On Diff #180455)

Yes, Characteristics is mandatory, otherwise it fails like this:

YAML:33:5: error: missing required key 'Characteristics'
59–61 ↗(On Diff #180455)

Yes, these fields are necessary.

78 ↗(On Diff #180455)

The whole SectionDefinition part can be omitted while keeping the test functioning.

80 ↗(On Diff #180455)

Can be omitted while keeping the test working.

mstorsjo updated this revision to Diff 180463.Jan 7 2019, 4:52 AM
mstorsjo marked 4 inline comments as done.

Stripped the test yaml further, with empty Characteristics and removed SectionDefinitions.

jhenderson accepted this revision.Jan 7 2019, 5:26 AM
This revision was automatically updated to reflect the committed changes.