Page MenuHomePhabricator

[llvm-tapi] Don't try to override SequenceTraits for std::string
ClosedPublic

Authored by sbc100 on Dec 6 2018, 11:39 AM.

Details

Summary

For some reason this doesn't seem to work with LLVM_LINK_LLVM_DYLIB
build.

See https://logs.chromium.org/logs/chromium/bb/client.wasm.llvm/linux/37764/+/recipes/steps/LLVM_regression_tests/0/stdout

What is more it seems that overriding these traits for core types
(including std::string) is not supported/recommend by YAMLTraits.h.
See line 1918 which has the assertion:
"only use LLVM_YAML_IS_SEQUENCE_VECTOR for types you control"

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Dec 6 2018, 11:39 AM
sbc100 retitled this revision from Don't try to override SequenceTraits for std::string to [llvm-tapi] Don't try to override SequenceTraits for std::string.Dec 6 2018, 11:39 AM
sbc100 added a reviewer: amontanez.

Apologies, I'll keep a closer eye on that in the future. This can be resolve by doing what ELFArch has done. This requires adding a typedef to TBEHandler.h, changing the declaration of NeededLibs to use the new typedef, and then modifying the ELFYAMLTest.cpp to use something like

LLVM_YAML_STRONG_TYPEDEF(ELFNeededList, ELFNeededMapper)

That should allow us to keep the flow mapping.

Apologies, I'll keep a closer eye on that in the future. This can be resolve by doing what ELFArch has done. This requires adding a typedef to TBEHandler.h, changing the declaration of NeededLibs to use the new typedef, and then modifying the ELFYAMLTest.cpp to use something like

LLVM_YAML_STRONG_TYPEDEF(ELFNeededList, ELFNeededMapper)

That should allow us to keep the flow mapping.

Nice! Do you want to do that or should I?

I can take care of that. I'll add you to the new patch.

sbc100 abandoned this revision.Dec 6 2018, 12:55 PM
amontanez added a comment.EditedDec 6 2018, 5:40 PM

My fix ended up triggering some warnings in the bots that don't really have a reasonable fix so I reverted it. This is probably the best solution for the time being. lgtm

My fix ended up triggering some warnings in the bots that don't really have a reasonable fix so I reverted it. This is probably the best solution for the time being. lgtm

So, whats the plan here?
ElfYamlTextAPI.YAMLWritesNoTBESyms continues to fail in my local builds with LLVM_LINK_LLVM_DYLIB...

I can open a new patch that does exactly this since this patch is marked as abandoned. I haven't checked to see if phabricator supports re-opening an abandoned change.

sbc100 reclaimed this revision.Dec 7 2018, 10:48 AM

Reopening as the alternative solution had issues.

No further comments, LGTM

amontanez accepted this revision.Dec 7 2018, 11:04 AM
This revision is now accepted and ready to land.Dec 7 2018, 11:04 AM
This revision was automatically updated to reflect the committed changes.