Skip to content

[CodeView] Flatten cmd args in frontend for LF_BUILDINFO #106369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

nebulark
Copy link
Contributor

This change moves the flattening of commandline arguments for LF_BUILDINFO into the compiler frontend. This way other compiler frontends that use LLVM can do that flattening in a way that makes sense for them. E.g. clang wants to ensure a "-cc1" argument exists, while other frontends (e.g. rustc) don't.

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. debuginfo labels Aug 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 28, 2024

@llvm/pr-subscribers-clang-codegen

Author: None (nebulark)

Changes

This change moves the flattening of commandline arguments for LF_BUILDINFO into the compiler frontend. This way other compiler frontends that use LLVM can do that flattening in a way that makes sense for them. E.g. clang wants to ensure a "-cc1" argument exists, while other frontends (e.g. rustc) don't.


Full diff: https://github.com/llvm/llvm-project/pull/106369.diff

3 Files Affected:

  • (modified) clang/lib/CodeGen/BackendUtil.cpp (+38)
  • (modified) llvm/include/llvm/MC/MCTargetOptions.h (+7)
  • (modified) llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp (+12-3)
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index e765bbf637a661..cc6bb5824cbe98 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -50,6 +50,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/ToolOutputFile.h"
@@ -321,6 +322,41 @@ static bool actionRequiresCodeGen(BackendAction Action) {
          Action != Backend_EmitLL;
 }
 
+static std::string flattenClangCommandLine(ArrayRef<std::string> Args,
+                                      StringRef MainFilename) {
+  std::string FlatCmdLine;
+  raw_string_ostream OS(FlatCmdLine);
+  bool PrintedOneArg = false;
+  if (!StringRef(Args[0]).contains("-cc1")) {
+    llvm::sys::printArg(OS, "-cc1", /*Quote=*/true);
+    PrintedOneArg = true;
+  }
+  for (unsigned i = 0; i < Args.size(); i++) {
+    StringRef Arg = Args[i];
+    if (Arg.empty()) {
+      continue;
+    }
+    if (Arg == "-main-file-name" || Arg == "-o") {
+      i++; // Skip this argument and next one.
+      continue;
+    }
+    if (Arg.starts_with("-object-file-name") || Arg == MainFilename) {
+      continue;
+    }
+    // Skip fmessage-length for reproduciability.
+    if (Arg.starts_with("-fmessage-length")) {
+      continue;
+    }
+    if (PrintedOneArg) {
+      OS << " ";
+    }
+    llvm::sys::printArg(OS, Arg, /*Quote=*/true);
+    PrintedOneArg = true;
+  }
+  OS.flush();
+  return FlatCmdLine;
+}
+
 static bool initTargetOptions(DiagnosticsEngine &Diags,
                               llvm::TargetOptions &Options,
                               const CodeGenOptions &CodeGenOpts,
@@ -484,6 +520,8 @@ static bool initTargetOptions(DiagnosticsEngine &Diags,
           Entry.IgnoreSysRoot ? Entry.Path : HSOpts.Sysroot + Entry.Path);
   Options.MCOptions.Argv0 = CodeGenOpts.Argv0;
   Options.MCOptions.CommandLineArgs = CodeGenOpts.CommandLineArgs;
+  Options.MCOptions.CommandlineArgsFlat = flattenClangCommandLine(
+      CodeGenOpts.CommandLineArgs, CodeGenOpts.MainFileName);
   Options.MCOptions.AsSecureLogFile = CodeGenOpts.AsSecureLogFile;
   Options.MCOptions.PPCUseFullRegisterNames =
       CodeGenOpts.PPCUseFullRegisterNames;
diff --git a/llvm/include/llvm/MC/MCTargetOptions.h b/llvm/include/llvm/MC/MCTargetOptions.h
index 98317712250bf9..f4a002c82ef329 100644
--- a/llvm/include/llvm/MC/MCTargetOptions.h
+++ b/llvm/include/llvm/MC/MCTargetOptions.h
@@ -92,8 +92,15 @@ class MCTargetOptions {
   std::string AsSecureLogFile;
 
   const char *Argv0 = nullptr;
+
+  // Deprecated: Use FlatCommandlineArgs instead
+  // Arguments here are interpreted as coming from clang, formated and end up in LF_BUILDINFO as CommandLineArgs 
   ArrayRef<std::string> CommandLineArgs;
 
+  // Arguments here end up in LF_BUILDINFO as CommandLineArgs without any additional formating
+  // If empty falls back to CommandLineArgs
+  std::string CommandlineArgsFlat;
+
   /// Additional paths to search for `.include` directives when using the
   /// integrated assembler.
   std::vector<std::string> IASSearchPaths;
diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
index dddc08b3bc0166..f95a8f1c428b86 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
@@ -893,6 +893,8 @@ static TypeIndex getStringIdTypeIdx(GlobalTypeTableBuilder &TypeTable,
   return TypeTable.writeLeafType(SIR);
 }
 
+// This just exists for backwards compatability for the deprecated MCTargetOptions::CommandLineArgs
+// It assumed a clang compiler frontend
 static std::string flattenCommandLine(ArrayRef<std::string> Args,
                                       StringRef MainFilename) {
   std::string FlatCmdLine;
@@ -950,9 +952,16 @@ void CodeViewDebug::emitBuildInfo() {
   if (Asm->TM.Options.MCOptions.Argv0 != nullptr) {
     BuildInfoArgs[BuildInfoRecord::BuildTool] =
         getStringIdTypeIdx(TypeTable, Asm->TM.Options.MCOptions.Argv0);
-    BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
-        TypeTable, flattenCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
-                                      MainSourceFile->getFilename()));
+
+    if (!Asm->TM.Options.MCOptions.CommandlineArgsFlat.empty()) {
+      BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+          TypeTable, Asm->TM.Options.MCOptions.CommandlineArgsFlat);
+    } else {
+      BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+          TypeTable,
+          flattenCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
+                             MainSourceFile->getFilename()));
+    }
   }
   BuildInfoRecord BIR(BuildInfoArgs);
   TypeIndex BuildInfoIndex = TypeTable.writeLeafType(BIR);

@llvmbot
Copy link
Member

llvmbot commented Aug 28, 2024

@llvm/pr-subscribers-clang

Author: None (nebulark)

Changes

This change moves the flattening of commandline arguments for LF_BUILDINFO into the compiler frontend. This way other compiler frontends that use LLVM can do that flattening in a way that makes sense for them. E.g. clang wants to ensure a "-cc1" argument exists, while other frontends (e.g. rustc) don't.


Full diff: https://github.com/llvm/llvm-project/pull/106369.diff

3 Files Affected:

  • (modified) clang/lib/CodeGen/BackendUtil.cpp (+38)
  • (modified) llvm/include/llvm/MC/MCTargetOptions.h (+7)
  • (modified) llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp (+12-3)
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index e765bbf637a661..cc6bb5824cbe98 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -50,6 +50,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/ToolOutputFile.h"
@@ -321,6 +322,41 @@ static bool actionRequiresCodeGen(BackendAction Action) {
          Action != Backend_EmitLL;
 }
 
+static std::string flattenClangCommandLine(ArrayRef<std::string> Args,
+                                      StringRef MainFilename) {
+  std::string FlatCmdLine;
+  raw_string_ostream OS(FlatCmdLine);
+  bool PrintedOneArg = false;
+  if (!StringRef(Args[0]).contains("-cc1")) {
+    llvm::sys::printArg(OS, "-cc1", /*Quote=*/true);
+    PrintedOneArg = true;
+  }
+  for (unsigned i = 0; i < Args.size(); i++) {
+    StringRef Arg = Args[i];
+    if (Arg.empty()) {
+      continue;
+    }
+    if (Arg == "-main-file-name" || Arg == "-o") {
+      i++; // Skip this argument and next one.
+      continue;
+    }
+    if (Arg.starts_with("-object-file-name") || Arg == MainFilename) {
+      continue;
+    }
+    // Skip fmessage-length for reproduciability.
+    if (Arg.starts_with("-fmessage-length")) {
+      continue;
+    }
+    if (PrintedOneArg) {
+      OS << " ";
+    }
+    llvm::sys::printArg(OS, Arg, /*Quote=*/true);
+    PrintedOneArg = true;
+  }
+  OS.flush();
+  return FlatCmdLine;
+}
+
 static bool initTargetOptions(DiagnosticsEngine &Diags,
                               llvm::TargetOptions &Options,
                               const CodeGenOptions &CodeGenOpts,
@@ -484,6 +520,8 @@ static bool initTargetOptions(DiagnosticsEngine &Diags,
           Entry.IgnoreSysRoot ? Entry.Path : HSOpts.Sysroot + Entry.Path);
   Options.MCOptions.Argv0 = CodeGenOpts.Argv0;
   Options.MCOptions.CommandLineArgs = CodeGenOpts.CommandLineArgs;
+  Options.MCOptions.CommandlineArgsFlat = flattenClangCommandLine(
+      CodeGenOpts.CommandLineArgs, CodeGenOpts.MainFileName);
   Options.MCOptions.AsSecureLogFile = CodeGenOpts.AsSecureLogFile;
   Options.MCOptions.PPCUseFullRegisterNames =
       CodeGenOpts.PPCUseFullRegisterNames;
diff --git a/llvm/include/llvm/MC/MCTargetOptions.h b/llvm/include/llvm/MC/MCTargetOptions.h
index 98317712250bf9..f4a002c82ef329 100644
--- a/llvm/include/llvm/MC/MCTargetOptions.h
+++ b/llvm/include/llvm/MC/MCTargetOptions.h
@@ -92,8 +92,15 @@ class MCTargetOptions {
   std::string AsSecureLogFile;
 
   const char *Argv0 = nullptr;
+
+  // Deprecated: Use FlatCommandlineArgs instead
+  // Arguments here are interpreted as coming from clang, formated and end up in LF_BUILDINFO as CommandLineArgs 
   ArrayRef<std::string> CommandLineArgs;
 
+  // Arguments here end up in LF_BUILDINFO as CommandLineArgs without any additional formating
+  // If empty falls back to CommandLineArgs
+  std::string CommandlineArgsFlat;
+
   /// Additional paths to search for `.include` directives when using the
   /// integrated assembler.
   std::vector<std::string> IASSearchPaths;
diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
index dddc08b3bc0166..f95a8f1c428b86 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
@@ -893,6 +893,8 @@ static TypeIndex getStringIdTypeIdx(GlobalTypeTableBuilder &TypeTable,
   return TypeTable.writeLeafType(SIR);
 }
 
+// This just exists for backwards compatability for the deprecated MCTargetOptions::CommandLineArgs
+// It assumed a clang compiler frontend
 static std::string flattenCommandLine(ArrayRef<std::string> Args,
                                       StringRef MainFilename) {
   std::string FlatCmdLine;
@@ -950,9 +952,16 @@ void CodeViewDebug::emitBuildInfo() {
   if (Asm->TM.Options.MCOptions.Argv0 != nullptr) {
     BuildInfoArgs[BuildInfoRecord::BuildTool] =
         getStringIdTypeIdx(TypeTable, Asm->TM.Options.MCOptions.Argv0);
-    BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
-        TypeTable, flattenCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
-                                      MainSourceFile->getFilename()));
+
+    if (!Asm->TM.Options.MCOptions.CommandlineArgsFlat.empty()) {
+      BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+          TypeTable, Asm->TM.Options.MCOptions.CommandlineArgsFlat);
+    } else {
+      BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+          TypeTable,
+          flattenCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
+                             MainSourceFile->getFilename()));
+    }
   }
   BuildInfoRecord BIR(BuildInfoArgs);
   TypeIndex BuildInfoIndex = TypeTable.writeLeafType(BIR);

@llvmbot
Copy link
Member

llvmbot commented Aug 28, 2024

@llvm/pr-subscribers-debuginfo

Author: None (nebulark)

Changes

This change moves the flattening of commandline arguments for LF_BUILDINFO into the compiler frontend. This way other compiler frontends that use LLVM can do that flattening in a way that makes sense for them. E.g. clang wants to ensure a "-cc1" argument exists, while other frontends (e.g. rustc) don't.


Full diff: https://github.com/llvm/llvm-project/pull/106369.diff

3 Files Affected:

  • (modified) clang/lib/CodeGen/BackendUtil.cpp (+38)
  • (modified) llvm/include/llvm/MC/MCTargetOptions.h (+7)
  • (modified) llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp (+12-3)
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index e765bbf637a661..cc6bb5824cbe98 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -50,6 +50,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/ToolOutputFile.h"
@@ -321,6 +322,41 @@ static bool actionRequiresCodeGen(BackendAction Action) {
          Action != Backend_EmitLL;
 }
 
+static std::string flattenClangCommandLine(ArrayRef<std::string> Args,
+                                      StringRef MainFilename) {
+  std::string FlatCmdLine;
+  raw_string_ostream OS(FlatCmdLine);
+  bool PrintedOneArg = false;
+  if (!StringRef(Args[0]).contains("-cc1")) {
+    llvm::sys::printArg(OS, "-cc1", /*Quote=*/true);
+    PrintedOneArg = true;
+  }
+  for (unsigned i = 0; i < Args.size(); i++) {
+    StringRef Arg = Args[i];
+    if (Arg.empty()) {
+      continue;
+    }
+    if (Arg == "-main-file-name" || Arg == "-o") {
+      i++; // Skip this argument and next one.
+      continue;
+    }
+    if (Arg.starts_with("-object-file-name") || Arg == MainFilename) {
+      continue;
+    }
+    // Skip fmessage-length for reproduciability.
+    if (Arg.starts_with("-fmessage-length")) {
+      continue;
+    }
+    if (PrintedOneArg) {
+      OS << " ";
+    }
+    llvm::sys::printArg(OS, Arg, /*Quote=*/true);
+    PrintedOneArg = true;
+  }
+  OS.flush();
+  return FlatCmdLine;
+}
+
 static bool initTargetOptions(DiagnosticsEngine &Diags,
                               llvm::TargetOptions &Options,
                               const CodeGenOptions &CodeGenOpts,
@@ -484,6 +520,8 @@ static bool initTargetOptions(DiagnosticsEngine &Diags,
           Entry.IgnoreSysRoot ? Entry.Path : HSOpts.Sysroot + Entry.Path);
   Options.MCOptions.Argv0 = CodeGenOpts.Argv0;
   Options.MCOptions.CommandLineArgs = CodeGenOpts.CommandLineArgs;
+  Options.MCOptions.CommandlineArgsFlat = flattenClangCommandLine(
+      CodeGenOpts.CommandLineArgs, CodeGenOpts.MainFileName);
   Options.MCOptions.AsSecureLogFile = CodeGenOpts.AsSecureLogFile;
   Options.MCOptions.PPCUseFullRegisterNames =
       CodeGenOpts.PPCUseFullRegisterNames;
diff --git a/llvm/include/llvm/MC/MCTargetOptions.h b/llvm/include/llvm/MC/MCTargetOptions.h
index 98317712250bf9..f4a002c82ef329 100644
--- a/llvm/include/llvm/MC/MCTargetOptions.h
+++ b/llvm/include/llvm/MC/MCTargetOptions.h
@@ -92,8 +92,15 @@ class MCTargetOptions {
   std::string AsSecureLogFile;
 
   const char *Argv0 = nullptr;
+
+  // Deprecated: Use FlatCommandlineArgs instead
+  // Arguments here are interpreted as coming from clang, formated and end up in LF_BUILDINFO as CommandLineArgs 
   ArrayRef<std::string> CommandLineArgs;
 
+  // Arguments here end up in LF_BUILDINFO as CommandLineArgs without any additional formating
+  // If empty falls back to CommandLineArgs
+  std::string CommandlineArgsFlat;
+
   /// Additional paths to search for `.include` directives when using the
   /// integrated assembler.
   std::vector<std::string> IASSearchPaths;
diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
index dddc08b3bc0166..f95a8f1c428b86 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
@@ -893,6 +893,8 @@ static TypeIndex getStringIdTypeIdx(GlobalTypeTableBuilder &TypeTable,
   return TypeTable.writeLeafType(SIR);
 }
 
+// This just exists for backwards compatability for the deprecated MCTargetOptions::CommandLineArgs
+// It assumed a clang compiler frontend
 static std::string flattenCommandLine(ArrayRef<std::string> Args,
                                       StringRef MainFilename) {
   std::string FlatCmdLine;
@@ -950,9 +952,16 @@ void CodeViewDebug::emitBuildInfo() {
   if (Asm->TM.Options.MCOptions.Argv0 != nullptr) {
     BuildInfoArgs[BuildInfoRecord::BuildTool] =
         getStringIdTypeIdx(TypeTable, Asm->TM.Options.MCOptions.Argv0);
-    BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
-        TypeTable, flattenCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
-                                      MainSourceFile->getFilename()));
+
+    if (!Asm->TM.Options.MCOptions.CommandlineArgsFlat.empty()) {
+      BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+          TypeTable, Asm->TM.Options.MCOptions.CommandlineArgsFlat);
+    } else {
+      BuildInfoArgs[BuildInfoRecord::CommandLine] = getStringIdTypeIdx(
+          TypeTable,
+          flattenCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
+                             MainSourceFile->getFilename()));
+    }
   }
   BuildInfoRecord BIR(BuildInfoArgs);
   TypeIndex BuildInfoIndex = TypeTable.writeLeafType(BIR);

@@ -92,8 +92,15 @@ class MCTargetOptions {
std::string AsSecureLogFile;

const char *Argv0 = nullptr;

// Deprecated: Use FlatCommandlineArgs instead
// Arguments here are interpreted as coming from clang, formated and end up in LF_BUILDINFO as CommandLineArgs
ArrayRef<std::string> CommandLineArgs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, I am fine keeping just the flat arguments.

@@ -92,8 +92,15 @@ class MCTargetOptions {
std::string AsSecureLogFile;

const char *Argv0 = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind please moving (and adapting) the comment that you've added here above? Both Argv0 and CommandLineArgs are only used for storing a reproducible command in the Codeview debug infos.

@@ -893,6 +893,8 @@ static TypeIndex getStringIdTypeIdx(GlobalTypeTableBuilder &TypeTable,
return TypeTable.writeLeafType(SIR);
}

// This just exists for backwards compatability for the deprecated MCTargetOptions::CommandLineArgs
// It assumed a clang compiler frontend
static std::string flattenCommandLine(ArrayRef<std::string> Args,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this function. We'll let clients flatten their arguments as they wish. Please ensure that would work with rustc.

TypeTable, flattenCommandLine(Asm->TM.Options.MCOptions.CommandLineArgs,
MainSourceFile->getFilename()));

if (!Asm->TM.Options.MCOptions.CommandlineArgsFlat.empty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify, as per above comments.

@nebulark
Copy link
Contributor Author

Applied the suggested changes. I also made MCTargetOptions::Argv0 a std::string. This is more consistent with the rest and makes usage for other frontends (e.g. rustc) easier and removes the need for the null check.
It's better for cl and cmd in LF_BUILDINFO to always be set, as this has caused issues with some pdb tools. Even if those are just an empty string.

Copy link
Member

@aganea aganea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, but please correct the failing BuildKite tests (below) before we can go further.

if (Arg.starts_with("-object-file-name") || Arg == MainFilename) {
continue;
}
// Skip fmessage-length for reproduciability.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should read "reproducibility"

@@ -91,8 +91,11 @@ class MCTargetOptions {
std::string SplitDwarfFile;
std::string AsSecureLogFile;

const char *Argv0 = nullptr;
ArrayRef<std::string> CommandLineArgs;
// This will be set as compiler path in LF_BUILDINFO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to refer to "debug infos" and "Codeview" in the comment here, since MC is very generic. I would add a single comment, and group the two variables together.

@nebulark
Copy link
Contributor Author

Maded the suggested changes. Continuing looking into the failing tests.
This is my first llvm pr and I need some time to figure out the issues those are indicating and replicating those locally.

@nebulark nebulark force-pushed the flatten_compiler_args_in_frontend branch from 232e9b3 to 8977dbb Compare August 30, 2024 06:14
@aganea
Copy link
Member

aganea commented Aug 30, 2024

Maded the suggested changes. Continuing looking into the failing tests. This is my first llvm pr and I need some time to figure out the issues those are indicating and replicating those locally.

I recommend that you build a Debug target, ie. cmake -DCMAKE_BUILD_TYPE=Debug -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_ENABLE_ASSERTIONS=ON to ease debugging in Visual Studio. You can then run a specific test such as: py buildfolder\bin\llvm-lit.py -vv -a llvm/DebugInfo/COFF/types-data-members.ll. You'll see the commands executed for that test and their command-line, so you will be able to reproduce them.

I think in the specific case the tests are failing because the LF_BUILDINFO is somehow emitted slightly differently from what was there before your PR. Either fix the test files or fix the code to match what the tests are expecting.

This allows non-clang frontends use their own version. Made Arv0 owned.
Output empty string for commandline and Argv0 in LF_BUILDINFO, if those
are not set.
@nebulark nebulark force-pushed the flatten_compiler_args_in_frontend branch from e2921cc to e190d07 Compare September 2, 2024 20:21
Options.MCOptions.Argv0 = CodeGenOpts.Argv0;
Options.MCOptions.CommandLineArgs = CodeGenOpts.CommandLineArgs;
Options.MCOptions.Argv0 =
CodeGenOpts.Argv0 != nullptr ? CodeGenOpts.Argv0 : "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CodeGenOpts.Argv0 != nullptr ? CodeGenOpts.Argv0 : "";
CodeGenOpts.Argv0 ? CodeGenOpts.Argv0 : "";

@compnerd
Copy link
Member

compnerd commented Sep 5, 2024

Please also get a sign off from @aganea

@aganea
Copy link
Member

aganea commented Sep 5, 2024

The code LG but I’d like please @nebulark if you can explain de unit tests changes? Why those offset changes? The code should emit the same thing as before?

@nebulark
Copy link
Contributor Author

nebulark commented Sep 6, 2024

I always set Argv0 and Commandline, even if they are just an empty string. Some tools had issues if those were not set.
We already did use the empty string for BuildInfoRecord::TypeServerPDB, so now its also more consistent.

So the offset changed from 0x (null) to the empty string offset for some tests and is the same offset as the 4th entry (BuildInfoRecord::TypeServerPDB) in LF_BUILDINFO

@aganea
Copy link
Member

aganea commented Sep 6, 2024

I always set Argv0 and Commandline, even if they are just an empty string. Some tools had issues if those were not set. We already did use the empty string for BuildInfoRecord::TypeServerPDB, so now its also more consistent.

Ah yes, that's good. Looks good, thanks again!

@nebulark
Copy link
Contributor Author

Is there anything left to do?

@tru
Copy link
Collaborator

tru commented Sep 16, 2024

@nebulark do you have commit access to merge this? otherwise I can do it for you.

@nebulark
Copy link
Contributor Author

@tru No, this is my first llvm pr. I'd appreciate you doing it :)

@tru tru merged commit f5ba3e1 into llvm:main Sep 16, 2024
7 checks passed
@tru
Copy link
Collaborator

tru commented Sep 16, 2024

@tru No, this is my first llvm pr. I'd appreciate you doing it :)

Thanks for your contribution!

Copy link

@nebulark Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@nikic
Copy link
Contributor

nikic commented Sep 16, 2024

FYI this is a measurable compile-time regression when compiling tiny files (e.g. see the red 7zip entries at the end of https://llvm-compile-time-tracker.com/compare.php?from=5c348f692a8dff98a3780d0b859fb0949eccbaca&to=f5ba3e1fa6b5f862789786fbb4b342dfc2c27c33&stat=instructions%3Au&details=on). Not particularly concerning, but it seemed suspicious that formatting some command line arguments could have any measurable impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category debuginfo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants