[lld-macho] Process OSO prefix only textually in both input and output #152063
Conversation
The processing of `-oso_prefix` uses `llvm::sys::fs::real_path` from the user value, but it is later tried to be matched with the result of `make_absolute`. While `real_path` resolves special symbols like `.`, `..` and `~`, and resolves symlinks along the path, `make_absolute` does neither, causing an incompatibility in some situations. In macOS, temporary directories would normally be reported as `/var/folders/<random>`, but `/var` is in fact a symlink to `private/var`. If own is working on a temporary directory and uses `-oso_prefix .`, it will be expanded to `/private/var/folder/<random>`, while `make_absolute` will expand to `/var/folder/<random>` instead, and `-oso_prefix` will fail to remove the prefix from the `N_OSO` entries, leaving absolute paths to the temporary directory in the resulting file. This would happen in any situation in which the working directory includes a symlink, not only in temporary directories. One can change the usage of `make_absolute` to use `real_path` as well, but `real_path` will mean checking the file system for each `N_OSO` entry. The other solution is stop using `real_path` when processing `-oso_prefix` and manually expand an input of `.` like `make_absolute` will do. This second option is the one implemented here, since it is the closest to the visible behaviour of ld64 (like the removed comment notes), so it is the better one for compatibility. This means that a test that checked the usage of the tilde as `-oso_prefix` needs to be removed (since it was done by using `real_path`), and two new tests are provided checking that symlinks do not affect the result. The second test checks a change in behaviour, in which if one provides the input files with a prefix of `./`, even when using `-oso_prefix .` because the matching is textual, the `./` prefix will stay in the `N_OSO` entries. This matches the observed behaviour of ld64.
|
@llvm/pr-subscribers-lld Author: Daniel Rodríguez Troitiño (drodriguez) ChangesThe processing of In macOS, temporary directories would normally be reported as One can change the usage of This second option is the one implemented here, since it is the closest to the visible behaviour of ld64 (like the removed comment notes), so it is the better one for compatibility. This means that a test that checked the usage of the tilde as Full diff: https://github.com/llvm/llvm-project/pull/152063.diff 2 Files Affected:
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 9eb391c4ee1b9..0f3db4ffa2797 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -1635,27 +1635,23 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
config->osoPrefix = args.getLastArgValue(OPT_oso_prefix);
if (!config->osoPrefix.empty()) {
- // Expand special characters, such as ".", "..", or "~", if present.
- // Note: LD64 only expands "." and not other special characters.
- // That seems silly to imitate so we will not try to follow it, but rather
- // just use real_path() to do it.
-
// The max path length is 4096, in theory. However that seems quite long
// and seems unlikely that any one would want to strip everything from the
// path. Hence we've picked a reasonably large number here.
SmallString<1024> expanded;
- if (!fs::real_path(config->osoPrefix, expanded,
- /*expand_tilde=*/true)) {
- // Note: LD64 expands "." to be `<current_dir>/`
- // (ie., it has a slash suffix) whereas real_path() doesn't.
- // So we have to append '/' to be consistent.
- StringRef sep = sys::path::get_separator();
- // real_path removes trailing slashes as part of the normalization, but
- // these are meaningful for our text based stripping
- if (config->osoPrefix == "." || config->osoPrefix.ends_with(sep))
- expanded += sep;
- config->osoPrefix = saver().save(expanded.str());
+ // Expand "." into the current working directory.
+ if (config->osoPrefix == ".") {
+ if (!fs::current_path(expanded)) {
+ // Note: LD64 expands "." to be `<current_dir>/
+ // (ie., it has a slash suffix) whereas current_path() doesn't.
+ // So we have to append '/' to be consistent because this is
+ // meaningful for our text based stripping.
+ expanded += sys::path::get_separator();
+ }
+ } else {
+ expanded = config->osoPrefix;
}
+ config->osoPrefix = saver().save(expanded.str());
}
bool pie = args.hasFlag(OPT_pie, OPT_no_pie, true);
diff --git a/lld/test/MachO/stabs.s b/lld/test/MachO/stabs.s
index 968656dba5cba..e32b9fc5b50d6 100644
--- a/lld/test/MachO/stabs.s
+++ b/lld/test/MachO/stabs.s
@@ -63,9 +63,18 @@
# RUN: dsymutil -s %t/test-rel | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-PATH-NO-SLASH
# RUN: cd %t && ZERO_AR_DATE=0 %lld -lSystem test.o foo.o no-debug.o -oso_prefix "." -o %t/test-rel-dot
# RUN: dsymutil -s %t/test-rel-dot | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT
-## Set HOME to %t (for ~ to expand to)
-# RUN: cd %t && env HOME=%t ZERO_AR_DATE=0 %lld -lSystem test.o foo.o no-debug.o -oso_prefix "~" -o %t/test-rel-tilde
-# RUN: dsymutil -s %t/test-rel-tilde | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-PATH
+# RUN: cd %t && ZERO_AR_DATE=0 %lld -lSystem ./test.o ./foo.o ./no-debug.o -oso_prefix "." -o %t/test-rel-dot
+# RUN: dsymutil -s %t/test-rel-dot | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT-EXPLICIT
+
+## Check that symlinks are not expanded when -oso_prefix . is used.
+# RUN: mkdir -p %t/private/var/folders/tmp && ln -s private/var %t/var
+# RUN: cp %t/test.o %t/foo.o %t/no-debug.o %t/private/var/folders/tmp
+# RUN: env TZ=GMT touch -t "197001010000.16" %t/private/var/folders/tmp/test.o
+# RUN: env TZ=GMT touch -t "197001010000.32" %t/private/var/folders/tmp/foo.o
+# RUN: cd %t/var/folders/tmp && ZERO_AR_DATE=0 %lld -lSystem test.o foo.o no-debug.o -oso_prefix "." -o test-rel-symlink
+# RUN: dsymutil -s %t/private/var/folders/tmp/test-rel-symlink | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT
+# RUN: cd %t/var/folders/tmp && ZERO_AR_DATE=0 %lld -lSystem ./test.o ./foo.o ./no-debug.o -oso_prefix "." -o test-rel-symlink
+# RUN: dsymutil -s %t/private/var/folders/tmp/test-rel-symlink | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT-EXPLICIT
## Check that we don't emit DWARF or stabs when -S is used
# RUN: %lld -lSystem test.o foo.o no-debug.o -S -o %t/test-no-debug
@@ -91,6 +100,7 @@
# REL-PATH: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] '/test.o'
# REL-PATH-NO-SLASH: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] 'test.o'
# REL-DOT: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] 'test.o'
+# REL-DOT-EXPLICIT: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] './test.o'
# CHECK-NEXT: (N_STSYM ) [[#%.2d,MORE_DATA_ID + 1]] 0000 [[#%.16x,STATIC:]] '_static_var'
# CHECK-NEXT: (N_FUN ) [[#%.2d,TEXT_ID + 1]] 0000 [[#%.16x,MAIN:]] '_main'
# CHECK-NEXT: (N_FUN ) 00 0000 0000000000000006{{$}}
|
|
@llvm/pr-subscribers-lld-macho Author: Daniel Rodríguez Troitiño (drodriguez) ChangesThe processing of In macOS, temporary directories would normally be reported as One can change the usage of This second option is the one implemented here, since it is the closest to the visible behaviour of ld64 (like the removed comment notes), so it is the better one for compatibility. This means that a test that checked the usage of the tilde as Full diff: https://github.com/llvm/llvm-project/pull/152063.diff 2 Files Affected:
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 9eb391c4ee1b9..0f3db4ffa2797 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -1635,27 +1635,23 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
config->osoPrefix = args.getLastArgValue(OPT_oso_prefix);
if (!config->osoPrefix.empty()) {
- // Expand special characters, such as ".", "..", or "~", if present.
- // Note: LD64 only expands "." and not other special characters.
- // That seems silly to imitate so we will not try to follow it, but rather
- // just use real_path() to do it.
-
// The max path length is 4096, in theory. However that seems quite long
// and seems unlikely that any one would want to strip everything from the
// path. Hence we've picked a reasonably large number here.
SmallString<1024> expanded;
- if (!fs::real_path(config->osoPrefix, expanded,
- /*expand_tilde=*/true)) {
- // Note: LD64 expands "." to be `<current_dir>/`
- // (ie., it has a slash suffix) whereas real_path() doesn't.
- // So we have to append '/' to be consistent.
- StringRef sep = sys::path::get_separator();
- // real_path removes trailing slashes as part of the normalization, but
- // these are meaningful for our text based stripping
- if (config->osoPrefix == "." || config->osoPrefix.ends_with(sep))
- expanded += sep;
- config->osoPrefix = saver().save(expanded.str());
+ // Expand "." into the current working directory.
+ if (config->osoPrefix == ".") {
+ if (!fs::current_path(expanded)) {
+ // Note: LD64 expands "." to be `<current_dir>/
+ // (ie., it has a slash suffix) whereas current_path() doesn't.
+ // So we have to append '/' to be consistent because this is
+ // meaningful for our text based stripping.
+ expanded += sys::path::get_separator();
+ }
+ } else {
+ expanded = config->osoPrefix;
}
+ config->osoPrefix = saver().save(expanded.str());
}
bool pie = args.hasFlag(OPT_pie, OPT_no_pie, true);
diff --git a/lld/test/MachO/stabs.s b/lld/test/MachO/stabs.s
index 968656dba5cba..e32b9fc5b50d6 100644
--- a/lld/test/MachO/stabs.s
+++ b/lld/test/MachO/stabs.s
@@ -63,9 +63,18 @@
# RUN: dsymutil -s %t/test-rel | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-PATH-NO-SLASH
# RUN: cd %t && ZERO_AR_DATE=0 %lld -lSystem test.o foo.o no-debug.o -oso_prefix "." -o %t/test-rel-dot
# RUN: dsymutil -s %t/test-rel-dot | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT
-## Set HOME to %t (for ~ to expand to)
-# RUN: cd %t && env HOME=%t ZERO_AR_DATE=0 %lld -lSystem test.o foo.o no-debug.o -oso_prefix "~" -o %t/test-rel-tilde
-# RUN: dsymutil -s %t/test-rel-tilde | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-PATH
+# RUN: cd %t && ZERO_AR_DATE=0 %lld -lSystem ./test.o ./foo.o ./no-debug.o -oso_prefix "." -o %t/test-rel-dot
+# RUN: dsymutil -s %t/test-rel-dot | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT-EXPLICIT
+
+## Check that symlinks are not expanded when -oso_prefix . is used.
+# RUN: mkdir -p %t/private/var/folders/tmp && ln -s private/var %t/var
+# RUN: cp %t/test.o %t/foo.o %t/no-debug.o %t/private/var/folders/tmp
+# RUN: env TZ=GMT touch -t "197001010000.16" %t/private/var/folders/tmp/test.o
+# RUN: env TZ=GMT touch -t "197001010000.32" %t/private/var/folders/tmp/foo.o
+# RUN: cd %t/var/folders/tmp && ZERO_AR_DATE=0 %lld -lSystem test.o foo.o no-debug.o -oso_prefix "." -o test-rel-symlink
+# RUN: dsymutil -s %t/private/var/folders/tmp/test-rel-symlink | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT
+# RUN: cd %t/var/folders/tmp && ZERO_AR_DATE=0 %lld -lSystem ./test.o ./foo.o ./no-debug.o -oso_prefix "." -o test-rel-symlink
+# RUN: dsymutil -s %t/private/var/folders/tmp/test-rel-symlink | grep 'N_OSO' | FileCheck %s -D#TEST_TIME=0x10 -D#FOO_TIME=0x20 --check-prefix=REL-DOT-EXPLICIT
## Check that we don't emit DWARF or stabs when -S is used
# RUN: %lld -lSystem test.o foo.o no-debug.o -S -o %t/test-no-debug
@@ -91,6 +100,7 @@
# REL-PATH: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] '/test.o'
# REL-PATH-NO-SLASH: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] 'test.o'
# REL-DOT: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] 'test.o'
+# REL-DOT-EXPLICIT: (N_OSO ) 03 0001 [[#%.16x,TEST_TIME]] './test.o'
# CHECK-NEXT: (N_STSYM ) [[#%.2d,MORE_DATA_ID + 1]] 0000 [[#%.16x,STATIC:]] '_static_var'
# CHECK-NEXT: (N_FUN ) [[#%.2d,TEXT_ID + 1]] 0000 [[#%.16x,MAIN:]] '_main'
# CHECK-NEXT: (N_FUN ) 00 0000 0000000000000006{{$}}
|
lld/MachO/Driver.cpp
Outdated
| config->osoPrefix = saver().save(expanded.str()); | ||
| // Expand "." into the current working directory. | ||
| if (config->osoPrefix == ".") { | ||
| if (!fs::current_path(expanded)) { |
There was a problem hiding this comment.
Do we want to silently ignore errors if it fails?
There was a problem hiding this comment.
About ignoring the error: it is basically the same flow when fs::real_path failed in the previous version. Nothing got expanded and the value was used as passed.
However, I think there's a logic difference that I introduced here, because config->osoPrefix will be overwritten with an empty expanded, so that might be a logic error that I need to fix. Thanks making me look this code more closely.
| expanded += sys::path::get_separator(); | ||
| } | ||
| } else { | ||
| expanded = config->osoPrefix; |
There was a problem hiding this comment.
Can you clarify why you're dropping the check for other special characters in the path? (ie., ~)
There was a problem hiding this comment.
The main reason is because ld64 doesn't do it, as you pointed in the comment.
The secondary reason is that
llvm-project/lld/MachO/SyntheticSections.cpp
Lines 1177 to 1196 in d478502
uses make_absolute, which does not do the expansion of ., ~ or the contraction of .., but tries to remove the post-processed value of -oso_prefix as a string, and that causes divergences when the value of real_path(".") and make_absolute(".") are not the same (like it happens when the current working directory is part of a symlinked folder). Either both sides use real_path or both sides use make_absolute, but one cannot use different functions in each side.
I preferred to use current_path here (which is what make_absolute uses underneath) to match both sides, which seems to be more similar to what ld64 does under the hood, and avoids the file system access that real_path will need for each path component.
The processing of
-oso_prefixusesllvm::sys::fs::real_pathfrom the user value, but it is later tried to be matched with the result ofmake_absolute. Whilereal_pathresolves special symbols like.,..and~, and resolves symlinks along the path,make_absolutedoes neither, causing an incompatibility in some situations.In macOS, temporary directories would normally be reported as
/var/folders/<random>, but/varis in fact a symlink toprivate/var. If own is working on a temporary directory and uses-oso_prefix ., it will be expanded to/private/var/folder/<random>, whilemake_absolutewill expand to/var/folder/<random>instead, and-oso_prefixwill fail to remove the prefix from theN_OSOentries, leaving absolute paths to the temporary directory in the resulting file. This would happen in any situation in which the working directory includes a symlink, not only in temporary directories.One can change the usage of
make_absoluteto usereal_pathas well, butreal_pathwill mean checking the file system for eachN_OSOentry. The other solution is stop usingreal_pathwhen processing-oso_prefixand manually expand an input of.likemake_absolutewill do.This second option is the one implemented here, since it is the closest to the visible behaviour of ld64 (like the removed comment notes), so it is the better one for compatibility. This means that a test that checked the usage of the tilde as
-oso_prefixneeds to be removed (since it was done by usingreal_path), and two new tests are provided checking that symlinks do not affect the result. The second test checks a change in behaviour, in which if one provides the input files with a prefix of./, even when using-oso_prefix .because the matching is textual, the./prefix will stay in theN_OSOentries. This matches the observed behaviour of ld64.