[PATCH] D111706: [lld-macho] Fix dangling string reference when adding frameworks

[Vy Nguyen via Phabricator via llvm-commits]

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