[lld] check cache before real_path in loadDylib #140791
Conversation
In llvm#137649 symlink resolution was added when loading dylibs. This introduced a performance regression when linking with a large number of inputs with LC_LOAD_DYLIB commands due to the syscall overhead of realpath. Refactor the change to be closer to the original: - first check if the given path is in the cache - if not, resolve it and check again - update cache entries of both paths to point to the same dylib This mitigates the regression as we do not incur the realpath cost for every loadDylib call, only once per unique path.
|
@llvm/pr-subscribers-lld-macho Author: Richard Howell (rmaz) ChangesIn #137649 symlink resolution was added when loading dylibs. This introduced a performance regression when linking with a large number of inputs with LC_LOAD_DYLIB commands due to the syscall overhead of realpath. Refactor the change to be closer to the original:
This mitigates the regression as we do not incur the realpath cost for every loadDylib call, only once per unique path. Full diff: https://github.com/llvm/llvm-project/pull/140791.diff 1 Files Affected:
diff --git a/lld/MachO/DriverUtils.cpp b/lld/MachO/DriverUtils.cpp
index cf874018fa34b..14d60eb4cfa81 100644
--- a/lld/MachO/DriverUtils.cpp
+++ b/lld/MachO/DriverUtils.cpp
@@ -229,12 +229,7 @@ static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
bool isBundleLoader, bool explicitlyLinked) {
- // Frameworks can be found from different symlink paths, so resolve
- // symlinks before looking up in the dylib cache.
- SmallString<128> realPath;
- std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
- CachedHashStringRef path(!err ? uniqueSaver().save(StringRef(realPath))
- : mbref.getBufferIdentifier());
+ CachedHashStringRef path(mbref.getBufferIdentifier());
DylibFile *&file = loadedDylibs[path];
if (file) {
if (explicitlyLinked)
@@ -242,6 +237,23 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
return file;
}
+ // Frameworks can be found from different symlink paths, so resolve
+ // symlinks and look up in the dylib cache.
+ DylibFile *&realfile = file;
+ SmallString<128> realPath;
+ std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
+ if (!err) {
+ CachedHashStringRef resolvedPath(uniqueSaver().save(StringRef(realPath)));
+ realfile = loadedDylibs[resolvedPath];
+ if (realfile) {
+ if (explicitlyLinked)
+ realfile->setExplicitlyLinked();
+
+ file = realfile;
+ return realfile;
+ }
+ }
+
DylibFile *newFile;
file_magic magic = identify_magic(mbref.getBuffer());
if (magic == file_magic::tapi_file) {
@@ -253,6 +265,7 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
}
file =
make<DylibFile>(**result, umbrella, isBundleLoader, explicitlyLinked);
+ realfile = file;
// parseReexports() can recursively call loadDylib(). That's fine since
// we wrote the DylibFile we just loaded to the loadDylib cache via the
@@ -268,6 +281,7 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
magic == file_magic::macho_executable ||
magic == file_magic::macho_bundle);
file = make<DylibFile>(mbref, umbrella, isBundleLoader, explicitlyLinked);
+ realfile = file;
// parseLoadCommands() can also recursively call loadDylib(). See comment
// in previous block for why this means we must copy `file` here.
|
|
@llvm/pr-subscribers-lld Author: Richard Howell (rmaz) ChangesIn #137649 symlink resolution was added when loading dylibs. This introduced a performance regression when linking with a large number of inputs with LC_LOAD_DYLIB commands due to the syscall overhead of realpath. Refactor the change to be closer to the original:
This mitigates the regression as we do not incur the realpath cost for every loadDylib call, only once per unique path. Full diff: https://github.com/llvm/llvm-project/pull/140791.diff 1 Files Affected:
diff --git a/lld/MachO/DriverUtils.cpp b/lld/MachO/DriverUtils.cpp
index cf874018fa34b..14d60eb4cfa81 100644
--- a/lld/MachO/DriverUtils.cpp
+++ b/lld/MachO/DriverUtils.cpp
@@ -229,12 +229,7 @@ static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
bool isBundleLoader, bool explicitlyLinked) {
- // Frameworks can be found from different symlink paths, so resolve
- // symlinks before looking up in the dylib cache.
- SmallString<128> realPath;
- std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
- CachedHashStringRef path(!err ? uniqueSaver().save(StringRef(realPath))
- : mbref.getBufferIdentifier());
+ CachedHashStringRef path(mbref.getBufferIdentifier());
DylibFile *&file = loadedDylibs[path];
if (file) {
if (explicitlyLinked)
@@ -242,6 +237,23 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
return file;
}
+ // Frameworks can be found from different symlink paths, so resolve
+ // symlinks and look up in the dylib cache.
+ DylibFile *&realfile = file;
+ SmallString<128> realPath;
+ std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
+ if (!err) {
+ CachedHashStringRef resolvedPath(uniqueSaver().save(StringRef(realPath)));
+ realfile = loadedDylibs[resolvedPath];
+ if (realfile) {
+ if (explicitlyLinked)
+ realfile->setExplicitlyLinked();
+
+ file = realfile;
+ return realfile;
+ }
+ }
+
DylibFile *newFile;
file_magic magic = identify_magic(mbref.getBuffer());
if (magic == file_magic::tapi_file) {
@@ -253,6 +265,7 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
}
file =
make<DylibFile>(**result, umbrella, isBundleLoader, explicitlyLinked);
+ realfile = file;
// parseReexports() can recursively call loadDylib(). That's fine since
// we wrote the DylibFile we just loaded to the loadDylib cache via the
@@ -268,6 +281,7 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
magic == file_magic::macho_executable ||
magic == file_magic::macho_bundle);
file = make<DylibFile>(mbref, umbrella, isBundleLoader, explicitlyLinked);
+ realfile = file;
// parseLoadCommands() can also recursively call loadDylib(). See comment
// in previous block for why this means we must copy `file` here.
|
| SmallString<128> realPath; | ||
| std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath); | ||
| if (!err) { | ||
| CachedHashStringRef resolvedPath(uniqueSaver().save(StringRef(realPath))); |
There was a problem hiding this comment.
| CachedHashStringRef resolvedPath(uniqueSaver().save(StringRef(realPath))); | |
| CachedHashStringRef resolvedPath(uniqueSaver().save(realPath.str())); |
| @@ -253,6 +265,7 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella, | |||
| } | |||
| file = | |||
| make<DylibFile>(**result, umbrella, isBundleLoader, explicitlyLinked); | |||
| realfile = file; | |||
There was a problem hiding this comment.
In the common case we have
DylibFile *&file = loadedDylibs[path];
DylibFile *&realfile = file;
Then what happens on this line? It does a load and a store to the same address? I'm sure it does the right thing it just looks funny to me.
Also, if we set file later in the future, how can we make sure to not forget to also set realfile? Is there a better way of doing this?
There was a problem hiding this comment.
Then what happens on this line? It does a load and a store to the same address?
Thats what I thought would happen, yes.
Also, if we set file later in the future, how can we make sure to not forget to also set realfile? Is there a better way of doing this?
What would you suggest? Ultimately we have 2 cache pointers, that may or may not point to the same thing, and we need to update them.
There was a problem hiding this comment.
What if we just use realfile everywhere? In the case when the pointers are the same, everything works as expected. But when they are different, the first cache lookup is basically always a miss and will rely on the second resolved path lookup to find the result. In theory, it shouldn't be a huge hit, but it's still a minor regression from the current implementation. But, it makes things easier to reason about without needing to set both file and realfile?
There was a problem hiding this comment.
That would mean that we would never cache symlinks, only their resolved paths, which is the unhappy path. That would be a significant regression as a large number of load commands are symlinks (eg Foundation.framework/Foundation -> Foundation.framework/Versions/A/Foundation).
I also disagree with the idea of regressing the performance to reduce the amount of code by 2 lines.
The only alternative I can see is to do have realfile default to a nullptr and have the two setters change to:
if (realfile)
realfile = file;
Can't say I prefer it though.
There was a problem hiding this comment.
Would something like llvm::make_scope_exit([&]{ realfile = file; }) be OK to keep things organized? Would that work?
Edit: or add a single line after this if {} else {} block. I am scared that it will not be obvious there if something changes in the if {} else {} block, though.
This is causing use-after-frees due to references getting invalidating after the loadedDylibs map grows, see comments on the PR. This reverts commit 475a8a4.
There was a problem hiding this comment.
This is causing use-after-frees, see the comment above.
Moreover, there seems to be a deeper logic bug here: file and realfile both refer to the same value in the map. That does not seem to be the intention, as it means loadedDylibs[resolvedPath] will never get updated, so future callers may get the wrong value when looking it up.
These seems scary enough that we should revert, which I've done in c34351c.
| std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath); | ||
| if (!err) { | ||
| CachedHashStringRef resolvedPath(uniqueSaver().save(StringRef(realPath))); | ||
| realfile = loadedDylibs[resolvedPath]; |
There was a problem hiding this comment.
There's a subtle use-after-invalidation here. Both file and realfile are references to loadedDylibs[Path]. The right-hand-side may grow the map, invalidating realfile (and file), so when performing the assignment we get use-after-free.
See https://crbug.com/422206408 for a reproducer.
There was a problem hiding this comment.
Thanks for the repro and revert, looks like this is going to need a better approach.
| if (explicitlyLinked) | ||
| realfile->setExplicitlyLinked(); | ||
|
|
||
| file = realfile; |
There was a problem hiding this comment.
This is redundant since both file and realfile are references to loadedDylibs[Path].
As are all realfile = file assignments below.
There was a problem hiding this comment.
Thanks for the review.
At this point, in my head, file = loadedDylibs[Path], but realfile = loadedDylibs[resolvedPath]. Originally there was no DylibFile *&realfile = file; (it was directly assigned from loadedDylibs[resolvedPath]), which might change things, but I am not sure if it does.
The realfile = file; below was supposed to also make sure that the entry for Path was assigned the result assigned to resolvedPath as well, so both refer to the same actual DylibFile *.
There was a problem hiding this comment.
C++ references are sneaky. Once they're bound, they cannot be re-bound. I.e., after DylibFile *&realfile = file; any assignment to realfile will be like assigning to file.
There was a problem hiding this comment.
We should not have added that line then.
I found this about references to unordered_map being invalidated: https://eel.is/c++draft/unord.req#general-9 (emphasis mine)
Rehashing invalidates iterators, changes ordering between elements, and changes which buckets elements appear in, but does not invalidate pointers or references to elements
I am not sure if llvm::DenseMap follows all the rules of std::unordered_map, but that little bit might be something they forgot to keep compatible.
|
Hi! I am part of a toolchain team at Google. We have been seeing failures in MAC builders since the introduction of this patch. Some of the failures are due to linker crashes. I've gone as far as to verify that the failures disappear when we don't have this patch in our downstream tree. The nature of the failures vary and quite inconsistent to get to a reproducer yet. But my team is working on it. One such sample failure from our MAC ARM64 toolchain builder. There was another bug reported in the past few days which may be related as well but I am not fully sure if the reporter's toolchain had this patch on their tree. #143332 But we noticed similar crashes in our builders when this patch was in tree. @rmaz I will try to get more information but it would help if you can think of reasons that this patch could cause problems. |
|
Following up with more information. Some of the crash stack traces are listed in this bug: https://issues.fuchsia.dev/issues/421489509#comment4 |
This is causing use-after-frees due to references getting invalidating after the loadedDylibs map grows, see comments on the PR. This reverts commit 475a8a4.
This is causing use-after-frees due to references getting invalidating after the loadedDylibs map grows, see comments on the PR. This reverts commit 475a8a4.
In #137649 symlink resolution was added when loading dylibs. This introduced a performance regression when linking with a large number of inputs with LC_LOAD_DYLIB commands due to the syscall overhead of realpath.
Refactor the change to be closer to the original:
This mitigates the regression as we do not incur the realpath cost for every loadDylib call, only once per unique path.