[PATCH] D111706: [lld-macho] Fix dangling string reference when adding frameworks
Vy Nguyen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 18 07:22:40 PDT 2021
oontvoo added a comment.
In D111706#3069533 <https://reviews.llvm.org/D111706#3069533>, @PRESIDENT810 wrote:
> Hello.
>
> I added %t to paths in my test, and they should be ok now. I also changed a few functions related to adding frameworks, so now they return StringRef instances instead of std::string, and their values are all saved by saver.save(). In this way I think we don't need to change the key of loadArchives to hash_code.
Thanks! Looks good!
One last request, can you make the test use simplified asm (rather than yaml) ? The yaml format is kind of hairy and hard to maintain ... it's only used when the obj files cannot be produced any other way.
For this test, I don't think it's necessary to use it.
(There are other similar test in simplified asm, you may see https://github.com/llvm/llvm-project/tree/main/lld/test/MachO for egs).
================
Comment at: lld/MachO/Driver.cpp:116
- if (Optional<std::string> path = resolveDylibPath(symlink))
+ if (Optional<StringRef> path = resolveDylibPath(symlink))
return path;
----------------
I think this could be `resolveDylibPath(saver.save(symlink.str())`
Then in resolveDylibPath (ie., DriverUtils.cpp:line 199), you don't save the StringRef because the assumption is that when you see a StringRef, it's already been saved.
================
Comment at: lld/MachO/InputFiles.cpp:916
for (StringRef root : config->systemLibraryRoots)
- if (Optional<std::string> dylibPath =
+ if (Optional<StringRef> dylibPath =
resolveDylibPath((root + path).str()))
----------------
pls run clang-format on patch
================
Comment at: lld/MachO/InputFiles.cpp:917
+ if (Optional<StringRef> dylibPath =
resolveDylibPath((root + path).str()))
return loadDylib(*dylibPath, umbrella);
----------------
this should be saved too?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111706/new/
https://reviews.llvm.org/D111706
More information about the llvm-commits
mailing list