[DTLTO] Normalize Windows 8.3 short paths to long form #4
Conversation
This patch introduces support for Integrated Distributed ThinLTO (DTLTO) in ELF LLD. DTLTO enables the distribution of ThinLTO backend compilations via external distribution systems, such as Incredibuild, during the traditional link step: https://llvm.org/docs/DTLTO.html. It is expected that users will invoke DTLTO through the compiler driver (e.g., Clang) rather than calling LLD directly. A Clang-side interface for DTLTO will be added in a follow-up patch. Note: Bitcode members of non-thin archives are not currently supported. This will be addressed in a future change. Testing: - ELF LLD `lit` test coverage has been added, using a mock distributor to avoid requiring Clang. - Cross-project `lit` tests cover integration with Clang. For the design discussion of the DTLTO feature, see: llvm#126654
8.3-style short paths (e.g., C:\PROGRA~1) can cause inconsistencies or failures on remote machines during distributed linking. Such paths may not resolve correctly or may differ between systems. We anticipate that many/all distribution systems will have difficulties with such paths. This patch ensures that all Windows paths used for DTLTO in ELF LLD are converted to their long form using GetLongPathNameW. This avoids ambiguity and improves portability across most distribution systems.
bd1976bris
force-pushed
the
dtlto_normalize_paths
branch
from
2011c8c to
d24e531
Compare
There was a problem hiding this comment.
I have left some comments/questions.
In the commit comment, it says "We anticipate that many/all distribution systems will have difficulties with such paths". That's true, but we can additionally concretely say we know of at least one such system.
I think it's also worth saying that the occurrence of short form paths is quite common. One notable example is clang writing (temporary) objects to %TEMP%, which can be in short form.
llvm/unittests/Support/Path.cpp
Outdated
| if (!getFullPath(Path16, FullPath16)) | ||
| return false; |
There was a problem hiding this comment.
Can we instead use make_absolute while the path is still narrow? Then getFullPath() can be removed.
There was a problem hiding this comment.
Yes. Nice comment thanks very much. Inspired by this I tried using root_path and was able to get that to work. So we can drop the Win32 API wrappers for getting the full path and for getting the volume. We don't even need make_absoute because as this is test code we can simply guarantee that the incoming path is absolute.
There was a problem hiding this comment.
The python in the LIT config has been simplified in the same manner.
cross-project-tests/dtlto/path.test
Outdated
| # Copy the CWD to a unique directory inside TEMP and determine its 8.3 form. | ||
| # TEMP is likely to be on a drive that supports long+short paths. |
There was a problem hiding this comment.
This isn't guaranteed. So we ought to handle the case where short paths are not enabled.
There was a problem hiding this comment.
Yes. In fact I feel like this was addressed already and I have just dropped a commit somewhere along the line..
There was a problem hiding this comment.
Done now. I have used ctypes and python to query Win32s AreShortPathsEnabled function if it is available in the lit config for the cross-project tests.
Co-authored-by: Edd Dawson <[email protected]>
Co-authored-by: Edd Dawson <[email protected]>
Co-authored-by: Edd Dawson <[email protected]>
Co-authored-by: Edd Dawson <[email protected]>
Co-authored-by: Edd Dawson <[email protected]>
Co-authored-by: Edd Dawson <[email protected]>
The cross-project `path.test` assumes that the Windows volume for the %TEMP% directories has short paths enabled, this is not guaranteed. This change allows tests cross-project-tests to `REQUIRE: shortpaths` and `path.test` test uses this feature.
…nt with existing code.
Great points. I have updated the description of this PR to include those points and will use that description when I squash the commits here to create a single commit for LLVM side review. |
…etVolumePathNameW
…W rather than guessing the 8.3 short-form name
…ed. I wasn't able to figure out how to test this without admin permissions for the tests. Therefore, I'm not going to try to add automated tests for this.
…s expected. I wasn't able to figure out how to test this without admin permissions for the tests. Therefore, I'm not going to try to add automated tests for this." This reverts commit 7227c20.
Also: - Formatted all python with "black" - Minor test comment, naming and structure improvements.
bd1976bris
force-pushed
the
dtlto_elf_lld
branch
from
57ff3e0 to
da45393
Compare
8.3-style short-form paths (e.g., C:\PROGRA~1) can cause inconsistencies or
failures on remote machines during distributed linking. Such paths may not
resolve correctly or may differ between systems. Short-form paths are
relatively common - for example, clang often writes temporary object files
to %TEMP%, which is frequently a short-form path.
We are aware of at least one distribution system (SN-DBS) affected by this,
and we expect that many (perhaps all) distribution systems may encounter
similar issues.
This patch ensures that all Windows paths used for DTLTO in ELF LLD are
normalized to their long form using GetLongPathNameW. This avoids ambiguity
and improves portability across distribution environments.
For testing, AreShortNamesEnabled is used to detect whether 8.3 path support
is enabled on the current volume. However, this API is only available on
Windows 11. As a result, the related tests are skipped on earlier versions
of Windows due to the lack of a reliable alternative for feature detection.