[lld][ELF] Support LLVM repository and LLVM revision information #97323
Conversation
--version Added LLVM repository and LLVM revision information for ld.lld --version. The getLLVMRepositoryPath and getLLVMRevision are under lld/common so it should be availble for other drivers to use too. This change is only to ld.lld.
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-coff Author: Hongyu Chen (yugier) ChangesAdded LLVM repository and LLVM revision information for ld.lld --version. The getLLVMRepositoryPath and getLLVMRevision are under lld/common so it should be availble for other drivers to use too. This change is only to ld.lld. Before this change: After this change: Full diff: https://github.com/llvm/llvm-project/pull/97323.diff 4 Files Affected:
diff --git a/lld/Common/Version.cpp b/lld/Common/Version.cpp
index ec6eda6a6748f..da7485a4f28bb 100644
--- a/lld/Common/Version.cpp
+++ b/lld/Common/Version.cpp
@@ -11,8 +11,8 @@
//===----------------------------------------------------------------------===//
#include "lld/Common/Version.h"
-
#include "VCSVersion.inc"
+#include "llvm/Support/VCSRevision.h"
// Returns a version string, e.g.:
// LLD 14.0.0 (https://github.com/llvm/llvm-project.git
@@ -26,3 +26,19 @@ std::string lld::getLLDVersion() {
return LLD_VENDOR_DISPLAY "LLD " LLD_VERSION_STRING;
#undef LLD_VENDOR_DISPLAY
}
+
+std::string lld::getLLVMRepositoryPath() {
+#ifdef LLVM_REPOSITORY
+ return LLVM_REPOSITORY;
+#else
+ return "";
+#endif
+}
+
+std::string lld::getLLVMRevision() {
+#ifdef LLVM_REVISION
+ return LLVM_REVISION;
+#else
+ return "";
+#endif
+}
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index ed773f5e69f77..c8b658c9d2cd2 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -164,6 +164,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
return errorCount() == 0;
}
+
} // namespace elf
} // namespace lld
@@ -630,8 +631,10 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
// lot of "configure" scripts out there that are generated by old version
// of Libtool. We cannot convince every software developer to migrate to
// the latest version and re-generate scripts. So we have this hack.
- if (args.hasArg(OPT_v) || args.hasArg(OPT_version))
- message(getLLDVersion() + " (compatible with GNU linkers)");
+ if (args.hasArg(OPT_v) || args.hasArg(OPT_version)) {
+ message(getLLDVersion() + " (compatible with GNU linkers) " +
+ getLLVMRepositoryPath() + " " + getLLVMRevision());
+ }
if (const char *path = getReproduceOption(args)) {
// Note that --reproduce is a debug option so you can ignore it
diff --git a/lld/include/lld/Common/Version.h b/lld/include/lld/Common/Version.h
index 9571aa2743e5b..ec1bd590647f4 100644
--- a/lld/include/lld/Common/Version.h
+++ b/lld/include/lld/Common/Version.h
@@ -19,6 +19,8 @@
namespace lld {
/// Retrieves a string representing the complete lld version.
std::string getLLDVersion();
+std::string getLLVMRepositoryPath();
+std::string getLLVMRevision();
}
#endif // LLD_VERSION_H
diff --git a/lld/test/ELF/version.test b/lld/test/ELF/version.test
index cdeeb4795e185..ae4b145329767 100644
--- a/lld/test/ELF/version.test
+++ b/lld/test/ELF/version.test
@@ -7,4 +7,4 @@
# RUN: ld.lld -V 2>&1 | FileCheck %s
# RUN: not ld.lld -V %t/not-exist 2>&1 | FileCheck %s
-# CHECK: LLD {{.*}} (compatible with GNU linkers)
+# CHECK: LLD {{.*}} (compatible with GNU linkers) {{.*}} {{.*}}
|
lld/ELF/Driver.cpp
Outdated
| @@ -164,6 +164,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS, | |||
|
|
|||
| return errorCount() == 0; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Please remove empty line.
|
I think this change should instead be made to https://github.com/llvm/llvm-project/blob/f9efc295089217425d8220af892dcc5ede9eb9f7/lld/Common/Version.cpp so it applies to all LLD drivers uniformly. Specifically, the comment above llvm-project/lld/Common/Version.cpp Lines 17 to 19 in f9efc29 That's not the case though since the string doesn't include the repository or the revision information, so I'd change the implementation to match the comment. I'd also consider removing the parenthesis around the I think this is a bit cleaner than: |
|
I think that we should check autotools before changing the "(compatible with GNU linkers)" as it may have the parenthesis hardcoded. |
getLLDVersion didn't contain LLVM repository and LLVM revision information which does not match its comment. This commit updates it so it should return strings like `LLD 19.0.0 (https://github.com/yugier/llvm-project.git 4134b33) compatible with GNU linkers`
The rest of what it says does not matter. It is just checking for the string GNU. |
lld/Common/Version.cpp
Outdated
| #if defined(LLVM_REPOSITORY) || defined(LLVM_REVISION) | ||
| return LLD_VENDOR_DISPLAY "LLD " LLD_VERSION_STRING " (" LLVM_REPOSITORY | ||
| " " LLVM_REVISION ")"; | ||
| #endif | ||
| return LLD_VENDOR_DISPLAY "LLD " LLD_VERSION_STRING; |
There was a problem hiding this comment.
| #if defined(LLVM_REPOSITORY) || defined(LLVM_REVISION) | |
| return LLD_VENDOR_DISPLAY "LLD " LLD_VERSION_STRING " (" LLVM_REPOSITORY | |
| " " LLVM_REVISION ")"; | |
| #endif | |
| return LLD_VENDOR_DISPLAY "LLD " LLD_VERSION_STRING; | |
| #if defined(LLVM_REPOSITORY) || defined(LLVM_REVISION) | |
| return LLD_VENDOR_DISPLAY "LLD " LLD_VERSION_STRING " (" LLVM_REPOSITORY | |
| " " LLVM_REVISION ")"; | |
| #else | |
| return LLD_VENDOR_DISPLAY "LLD " LLD_VERSION_STRING; | |
| #endif |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
The description (which will be used as the default commit message when "Squash and merge" is clicked) is probably about
In the Without parentheses, "compatible ..." might be parsed as part of the version string. Should parentheses be added back? We could also use a leading comma or something else, but lld/MinGW/Driver.cpp should be updated as well for consistency. |
Looks like mold uses the following format: Should we use the same format for consistency? |
|
We could use a comma to be consistent with other LLVM binary utilities: |
Do you prefer we have or Thank you! |
|
|
Used a comma to seperate version information and "compatible with GNU linkers" in ELF and MinGW for being consistent with other LLVM binary utilities.
No problem! Updated! Please let me know if there is anything else that should be changed for this. |
I just did! Thank you for your fast response and help! :) |
…m#97323) Added LLVM repository and LLVM revision information for `lld::getLLDVersion()` Before this change: ``` hongyuchy@hongyuchy:~/llvm-project/.build_lld_version$ bin/ld.lld --version LLD 19.0.0 (compatible with GNU linkers) ``` After this change with LLVM_APPEND_VC_REV=on ``` hongyuchy@hongyuchy:~/llvm-project/.build_lld_version$ bin/ld.lld --version LLD 19.0.0 (https://github.com/yugier/llvm-project.git 4134b33), compatible with GNU linkers ``` with LLVM_APPEND_VC_REV=off ``` hongyuchy@hongyuchy:~/llvm-project/.build_lld_version$ bin/ld.lld --version LLD 19.0.0, compatible with GNU linkers ```
|
Sorry, but this change has broken how Meson detects and disambiguates between the I would request reinstating that form. We should of course request Meson to match a less strict pattern here as well, but even then, we should work with the variety of existing Meson versions out there as well, at least for a couple years after that has been changed. |
|
As this check in Meson only seems to be used to differentiate between the diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 1fd120ad3601..35fd478a2190 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -199,7 +199,7 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
// a GNU compatible linker. As long as an output for the -v option
// contains "GNU" or "with BFD", they recognize us as GNU-compatible.
if (args.hasArg(OPT_v) || args.hasArg(OPT_version))
- message(getLLDVersion() + ", compatible with GNU linkers");
+ message(getLLDVersion() + " (compatible with GNU linkers)");
// The behavior of -v or --version is a bit strange, but this is
// needed for compatibility with GNU linkers.
|
Don't require the string to be enclosed in parentheses. llvm/llvm-project#97323 changed the LLD version printout to no longer be enclosed in parentheses, which made Meson fail to detect the linker style used here. The LLD change is being reverted in llvm/llvm-project#97698 in order to fix building with existing Meson versions, but for the future, loosen the check slightly, to not require the parentheses.
|
I posted such a change in #97698, and made a PR to Meson to loosen the check now in mesonbuild/meson#13383. |
Don't require the string to be enclosed in parentheses. llvm/llvm-project#97323 changed the LLD version printout to no longer be enclosed in parentheses, which made Meson fail to detect the linker style used here. The LLD change is being reverted in llvm/llvm-project#97698 in order to fix building with existing Meson versions, but for the future, loosen the check slightly, to not require the parentheses.
…m#97323) Added LLVM repository and LLVM revision information for `lld::getLLDVersion()` Before this change: ``` hongyuchy@hongyuchy:~/llvm-project/.build_lld_version$ bin/ld.lld --version LLD 19.0.0 (compatible with GNU linkers) ``` After this change with LLVM_APPEND_VC_REV=on ``` hongyuchy@hongyuchy:~/llvm-project/.build_lld_version$ bin/ld.lld --version LLD 19.0.0 (https://github.com/yugier/llvm-project.git 4134b33), compatible with GNU linkers ``` with LLVM_APPEND_VC_REV=off ``` hongyuchy@hongyuchy:~/llvm-project/.build_lld_version$ bin/ld.lld --version LLD 19.0.0, compatible with GNU linkers ```
|
This change breaks scripts that want to detect the LLD version when I don't know if the Linux ones, etc. should be changed, but I would comment in passing that it can cause problems. |
With `LLVM_APPEND_VC_REV=off`, the new version message after #97323 looks like: ``` % /tmp/out/custom2/bin/ld.lld --version LLD 19.0.0, compatible with GNU linkers ``` A trailing comma after the version string might cause issues with version detection tools that don't strip it, as seen in the Linux kernel's scripts/ld-version.sh script. Pull Request: #97942
| @@ -23,6 +23,11 @@ std::string lld::getLLDVersion() { | |||
| #else | |||
| #define LLD_VENDOR_DISPLAY | |||
| #endif | |||
| #if defined(LLVM_REPOSITORY) || defined(LLVM_REVISION) | |||
There was a problem hiding this comment.
I think you meant && here.
Don't require the string to be enclosed in parentheses. llvm/llvm-project#97323 changed the LLD version printout to no longer be enclosed in parentheses, which made Meson fail to detect the linker style used here. The LLD change is being reverted in llvm/llvm-project#97698 in order to fix building with existing Meson versions, but for the future, loosen the check slightly, to not require the parentheses. (cherry picked from commit 1ca002a)
Don't require the string to be enclosed in parentheses. llvm/llvm-project#97323 changed the LLD version printout to no longer be enclosed in parentheses, which made Meson fail to detect the linker style used here. The LLD change is being reverted in llvm/llvm-project#97698 in order to fix building with existing Meson versions, but for the future, loosen the check slightly, to not require the parentheses.
Added LLVM repository and LLVM revision information for
lld::getLLDVersion()Before this change:
After this change with LLVM_APPEND_VC_REV=on
with LLVM_APPEND_VC_REV=off