https://github.com/jhuber6 created https://github.com/llvm/llvm-project/pull/183679
Summary: A bunch of small issues found through linting and LLM checking. - Broken sort comparator that violated strict weak ordering (UB) - SearchLibrary corrupting .lib filenames via erroneous drop_front() - Hardcoded x86_64-unknown-linux-gnu host triple in AMDGPU fatbinary - OffloadFile loop variable shadowing its own type, causing std::move on the type rather than the variable - GetDeviceInput calling exit() directly instead of returning Error - Redundant double-wrapping of DerivedArgList - Various typo and style fixes >From 20555c2011341e2986901ff7173e1218d105ee03 Mon Sep 17 00:00:00 2001 From: Joseph Huber <[email protected]> Date: Thu, 26 Feb 2026 21:51:16 -0600 Subject: [PATCH] [LinkerWrapper] Fix a bunch of minor issues and typos Summary: A bunch of small issues found through linting and LLM checking. - Broken sort comparator that violated strict weak ordering (UB) - SearchLibrary corrupting .lib filenames via erroneous drop_front() - Hardcoded x86_64-unknown-linux-gnu host triple in AMDGPU fatbinary - OffloadFile loop variable shadowing its own type, causing std::move on the type rather than the variable - GetDeviceInput calling exit() directly instead of returning Error - Redundant double-wrapping of DerivedArgList - Various typo and style fixes --- .../ClangLinkerWrapper.cpp | 87 +++++++++---------- 1 file changed, 43 insertions(+), 44 deletions(-) diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp index c49ce44432e5a..34cbbf58ab2e4 100644 --- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp +++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp @@ -1,10 +1,10 @@ -//===-- clang-linker-wrapper/ClangLinkerWrapper.cpp - wrapper over linker-===// +//===-- clang-linker-wrapper/ClangLinkerWrapper.cpp -----------------------===// // // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // See https://llvm.org/LICENSE.txt for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // -//===---------------------------------------------------------------------===// +//===----------------------------------------------------------------------===// // // This tool works as a wrapper over a linking job. This tool is used to create // linked device images for offloading. It scans the linker's input for embedded @@ -12,7 +12,7 @@ // as a temporary file. The extracted device files will then be passed to a // device linking job to create a final device image. // -//===---------------------------------------------------------------------===// +//===----------------------------------------------------------------------===// #include "clang/Basic/TargetID.h" #include "clang/Basic/Version.h" @@ -22,16 +22,12 @@ #include "llvm/CodeGen/CommandFlags.h" #include "llvm/Frontend/Offloading/OffloadWrapper.h" #include "llvm/Frontend/Offloading/Utility.h" -#include "llvm/IR/Constants.h" #include "llvm/IR/DiagnosticPrinter.h" #include "llvm/IR/Module.h" #include "llvm/IRReader/IRReader.h" #include "llvm/LTO/LTO.h" #include "llvm/MC/TargetRegistry.h" -#include "llvm/Object/Archive.h" -#include "llvm/Object/ArchiveWriter.h" #include "llvm/Object/Binary.h" -#include "llvm/Object/ELFObjectFile.h" #include "llvm/Object/IRObjectFile.h" #include "llvm/Object/ObjectFile.h" #include "llvm/Object/OffloadBinary.h" @@ -41,7 +37,6 @@ #include "llvm/Plugins/PassPlugin.h" #include "llvm/Remarks/HotnessThresholdParser.h" #include "llvm/Support/CommandLine.h" -#include "llvm/Support/Errc.h" #include "llvm/Support/FileOutputBuffer.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/InitLLVM.h" @@ -58,7 +53,6 @@ #include "llvm/Support/raw_ostream.h" #include "llvm/Target/TargetMachine.h" #include "llvm/TargetParser/Host.h" -#include <atomic> #include <optional> using namespace llvm; @@ -116,7 +110,7 @@ static cl::alias PassPipeline2("p", cl::aliasopt(PassPipeline), /// Path of the current binary. static const char *LinkerExecutable; -/// Ssave intermediary results. +/// Save intermediary results. static bool SaveTemps = false; /// Print arguments without executing. @@ -195,11 +189,8 @@ class WrapperOptTable : public opt::GenericOptTable { }; const OptTable &getOptTable() { - static const WrapperOptTable *Table = []() { - auto Result = std::make_unique<WrapperOptTable>(); - return Result.release(); - }(); - return *Table; + static const WrapperOptTable Table; + return Table; } void printCommands(ArrayRef<StringRef> CmdArgs) { @@ -218,10 +209,9 @@ void printCommands(ArrayRef<StringRef> CmdArgs) { exit(EXIT_FAILURE); } -std::string getMainExecutable(const char *Name) { - void *Ptr = (void *)(intptr_t)&getMainExecutable; - auto COWPath = sys::fs::getMainExecutable(Name, Ptr); - return sys::path::parent_path(COWPath).str(); +std::string getExecutableDir(const char *Name) { + void *Ptr = reinterpret_cast<void *>(&getExecutableDir); + return sys::path::parent_path(sys::fs::getMainExecutable(Name, Ptr)).str(); } /// Get a temporary filename suitable for output. @@ -263,7 +253,7 @@ Error executeCommands(StringRef ExecutablePath, ArrayRef<StringRef> Args) { if (!TempFileOrErr) return TempFileOrErr.takeError(); - SmallString<0> Contents; + SmallString<256> Contents; raw_svector_ostream OS(Contents); for (StringRef Arg : llvm::drop_begin(Args)) { sys::printArg(OS, Arg, /*Quote=*/true); @@ -321,7 +311,7 @@ Error relocateOffloadSection(const ArgList &Args, StringRef Output) { "Relocatable linking is not supported on COFF targets"); Expected<std::string> ObjcopyPath = - findProgram("llvm-objcopy", {getMainExecutable("llvm-objcopy")}); + findProgram("llvm-objcopy", {getExecutableDir("llvm-objcopy")}); if (!ObjcopyPath) return ObjcopyPath.takeError(); @@ -343,7 +333,7 @@ Error relocateOffloadSection(const ArgList &Args, StringRef Output) { ObjcopyArgs.emplace_back(".llvm.offloading"); StringRef Prefix = "llvm"; auto Section = (Prefix + "_offload_entries").str(); - // Rename the offloading entires to make them private to this link unit. + // Rename the offloading entries to make them private to this link unit. ObjcopyArgs.emplace_back("--rename-section"); ObjcopyArgs.emplace_back( Args.MakeArgString(Section + "=" + Section + Suffix)); @@ -381,7 +371,7 @@ Error runLinker(ArrayRef<StringRef> Files, const ArgList &Args) { Arg->render(Args, NewLinkerArgs); if (Arg->getOption().matches(OPT_o) || Arg->getOption().matches(OPT_out)) llvm::transform(Files, std::back_inserter(NewLinkerArgs), - [&](StringRef Arg) { return Args.MakeArgString(Arg); }); + [&](StringRef A) { return Args.MakeArgString(A); }); } SmallVector<StringRef> LinkerArgs({LinkerPath}); @@ -455,7 +445,7 @@ fatbinary(ArrayRef<std::tuple<StringRef, StringRef, StringRef>> InputFiles, // AMDGPU uses the clang-offload-bundler to bundle the linked images. Expected<std::string> OffloadBundlerPath = findProgram( - "clang-offload-bundler", {getMainExecutable("clang-offload-bundler")}); + "clang-offload-bundler", {getExecutableDir("clang-offload-bundler")}); if (!OffloadBundlerPath) return OffloadBundlerPath.takeError(); @@ -479,7 +469,10 @@ fatbinary(ArrayRef<std::tuple<StringRef, StringRef, StringRef>> InputFiles, CmdArgs.push_back( Args.MakeArgString(Twine("-compression-level=") + Arg->getValue())); - SmallVector<StringRef> Targets = {"-targets=host-x86_64-unknown-linux-gnu"}; + llvm::Triple HostTriple( + Args.getLastArgValue(OPT_host_triple_EQ, sys::getDefaultTargetTriple())); + SmallVector<StringRef> Targets = { + Saver.save("-targets=host-" + HostTriple.normalize())}; for (const auto &[File, TripleRef, Arch] : InputFiles) { std::string NormalizedTriple = normalizeForBundler(Triple(TripleRef), !Arch.empty()); @@ -510,7 +503,7 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args, llvm::TimeTraceScope TimeScope("Clang"); // Use `clang` to invoke the appropriate device tools. Expected<std::string> ClangPath = - findProgram("clang", {getMainExecutable("clang")}); + findProgram("clang", {getExecutableDir("clang")}); if (!ClangPath) return ClangPath.takeError(); @@ -547,7 +540,7 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args, if (Triple.isAMDGPU()) CmdArgs.push_back("-flto"); - // Forward all of the `--offload-opt` and similar options to the device. + // Forward all of the `--offload-opt` and `-mllvm` options to the device. for (auto &Arg : Args.filtered(OPT_offload_opt_eq_minus, OPT_mllvm)) CmdArgs.append( {"-Xlinker", @@ -897,10 +890,11 @@ bundleLinkedOutput(ArrayRef<OffloadingImage> Images, const ArgList &Args, } } -/// Returns a new ArgList containg arguments used for the device linking phase. +/// Returns a new ArgList containing arguments used for the device linking +/// phase. DerivedArgList getLinkerArgs(ArrayRef<OffloadFile> Input, const InputArgList &Args) { - DerivedArgList DAL = DerivedArgList(DerivedArgList(Args)); + DerivedArgList DAL(Args); for (Arg *A : Args) DAL.append(A); @@ -1090,9 +1084,11 @@ linkAndWrapDeviceFiles(ArrayRef<SmallVector<OffloadFile>> LinkerInputFiles, // We sort the entries before bundling so they appear in a deterministic // order in the final binary. llvm::sort(Input, [](OffloadingImage &A, OffloadingImage &B) { - return A.StringData["triple"] > B.StringData["triple"] || - A.StringData["arch"] > B.StringData["arch"] || - A.TheOffloadKind < B.TheOffloadKind; + if (A.StringData.lookup("triple") != B.StringData.lookup("triple")) + return A.StringData.lookup("triple") > B.StringData.lookup("triple"); + if (A.StringData.lookup("arch") != B.StringData.lookup("arch")) + return A.StringData.lookup("arch") > B.StringData.lookup("arch"); + return A.TheOffloadKind < B.TheOffloadKind; }); auto BundledImagesOrErr = bundleLinkedOutput(Input, Args, Kind); if (!BundledImagesOrErr) @@ -1166,8 +1162,10 @@ searchLibraryBaseName(StringRef Name, StringRef Root, /// `-lfoo` or `-l:libfoo.a`. std::optional<std::string> searchLibrary(StringRef Input, StringRef Root, ArrayRef<StringRef> SearchPaths) { - if (Input.starts_with(":") || Input.ends_with(".lib")) + if (Input.starts_with(":")) return findFromSearchPaths(Input.drop_front(), Root, SearchPaths); + if (Input.ends_with(".lib")) + return findFromSearchPaths(Input, Root, SearchPaths); return searchLibraryBaseName(Input, Root, SearchPaths); } @@ -1192,7 +1190,7 @@ getDeviceInput(const ArgList &Args) { StringSaver Saver(Alloc); // Try to extract device code from the linker input files. - bool WholeArchive = Args.hasArg(OPT_wholearchive_flag) ? true : false; + bool WholeArchive = Args.hasArg(OPT_wholearchive_flag); SmallVector<OffloadFile> ObjectFilesToExtract; SmallVector<OffloadFile> ArchiveFilesToExtract; for (const opt::Arg *Arg : Args.filtered( @@ -1209,8 +1207,7 @@ getDeviceInput(const ArgList &Args) { : std::string(Arg->getValue()); if (!Filename && Arg->getOption().matches(OPT_library)) - reportError( - createStringError("unable to find library -l%s", Arg->getValue())); + return createStringError("unable to find library -l%s", Arg->getValue()); if (!Filename || !sys::fs::exists(*Filename) || sys::fs::is_directory(*Filename)) @@ -1229,12 +1226,12 @@ getDeviceInput(const ArgList &Args) { if (Error Err = extractOffloadBinaries(Buffer, Binaries)) return std::move(Err); - for (auto &OffloadFile : Binaries) { + for (auto &Binary : Binaries) { if (identify_magic(Buffer.getBuffer()) == file_magic::archive && !WholeArchive) - ArchiveFilesToExtract.emplace_back(std::move(OffloadFile)); + ArchiveFilesToExtract.emplace_back(std::move(Binary)); else - ObjectFilesToExtract.emplace_back(std::move(OffloadFile)); + ObjectFilesToExtract.emplace_back(std::move(Binary)); } } @@ -1275,7 +1272,7 @@ getDeviceInput(const ArgList &Args) { CompatibleTargets.emplace_back(ID); for (const auto &[Index, ID] : llvm::enumerate(CompatibleTargets)) { - // Only extract an if we have an an object matching this target or it + // Only extract if we have an object matching this target or it // was specifically requested. if (!InputFiles.count(ID) && !ShouldExtract.contains(ID.second)) continue; @@ -1318,10 +1315,10 @@ int main(int Argc, char **Argv) { if (Args.hasArg(OPT_help) || Args.hasArg(OPT_help_hidden)) { Tbl.printHelp( outs(), - "clang-linker-wrapper [options] -- <options to passed to the linker>", + "clang-linker-wrapper [options] -- <options to pass to the linker>", "\nA wrapper utility over the host linker. It scans the input files\n" "for sections that require additional processing prior to linking.\n" - "The will then transparently pass all arguments and input to the\n" + "It will then transparently pass all arguments and input to the\n" "specified host linker to create the final binary.\n", Args.hasArg(OPT_help_hidden), Args.hasArg(OPT_help_hidden)); return EXIT_SUCCESS; @@ -1378,8 +1375,10 @@ int main(int Argc, char **Argv) { if (Args.hasArg(OPT_wrapper_time_trace_eq)) { unsigned Granularity; - Args.getLastArgValue(OPT_wrapper_time_trace_granularity, "500") - .getAsInteger(10, Granularity); + if (Args.getLastArgValue(OPT_wrapper_time_trace_granularity, "500") + .getAsInteger(10, Granularity)) + reportError( + createStringError("invalid value for time trace granularity")); timeTraceProfilerInitialize(Granularity, Argv[0]); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
