Skip to content
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

[Support] Trim trailing null character in PrettyStackTraceFormat. #77351

Closed
wants to merge 1 commit into from

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jan 8, 2024

Space for the \0 is added on line 247, but the value is not trimmed. SmallVector prints the entire contents, including the \0. To fix, pop the back value to remove the \0.

Sample program, and cat -v can be used to show the null (as ^@):

#include "llvm/Support/PrettyStackTrace.h"

auto main() -> int {
  auto msg = llvm::PrettyStackTraceFormat("x");
  llvm::EnablePrettyStackTrace();
  assert(false);
}

Before:

$ ./crash_test |& cat -v
crash_test: toolchain/driver/crash_test.cpp:6: int main(): Assertion `false' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	x^@

After:

$ ./crash_test |& cat -v
crash_test: toolchain/driver/crash_test.cpp:6: int main(): Assertion `false' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	x

Fixes #81273

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 8, 2024

@llvm/pr-subscribers-llvm-support

Author: Jon Ross-Perkins (jonmeow)

Changes

Space for the \0 is added on line 247, but the value is not trimmed. SmallVector prints the entire contents, including the \0. To fix, pop the back value to remove the \0.

Sample program, and cat -v can be used to show the null (as ^@):

#include "llvm/Support/PrettyStackTrace.h"

auto main() -> int {
  auto msg = llvm::PrettyStackTraceFormat("x");
  llvm::EnablePrettyStackTrace();
  assert(false);
}

Before:

$ ./crash_test |& cat -v
crash_test: toolchain/driver/crash_test.cpp:6: int main(): Assertion `false' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	x^@

After:

$ ./crash_test |& cat -v
crash_test: toolchain/driver/crash_test.cpp:6: int main(): Assertion `false' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	x

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

1 Files Affected:

  • (modified) llvm/lib/Support/PrettyStackTrace.cpp (+2)
diff --git a/llvm/lib/Support/PrettyStackTrace.cpp b/llvm/lib/Support/PrettyStackTrace.cpp
index f9f1b8a419b820..fc3e08e3662217 100644
--- a/llvm/lib/Support/PrettyStackTrace.cpp
+++ b/llvm/lib/Support/PrettyStackTrace.cpp
@@ -249,6 +249,8 @@ PrettyStackTraceFormat::PrettyStackTraceFormat(const char *Format, ...) {
   va_start(AP, Format);
   vsnprintf(Str.data(), Size, Format, AP);
   va_end(AP);
+  // Remove the '\0' for printing.
+  Str.pop_back();
 }
 
 void PrettyStackTraceFormat::print(raw_ostream &OS) const { OS << Str << "\n"; }

@jonmeow jonmeow force-pushed the clang-tidy branch 2 times, most recently from 06255e2 to e2f53f9 Compare January 23, 2024 17:22
@jonmeow
Copy link
Contributor Author

jonmeow commented Jan 25, 2024

AFAICT the buildkite issues aren't related, but I'm having trouble getting them to cleanly run. Is there something I should be doing here for review?

github-merge-queue bot pushed a commit to carbon-language/carbon-lang that referenced this pull request Feb 12, 2024
When we always had a single file in the test file, we could run the
driver over it to see output. This adds the ability to do something
similar for multi-file tests. So for example in test output, there's
now:

```
[ RUN      ] ToolchainFileTest.toolchain/check/testdata/class/fail_todo_import.carbon

To test this file alone, run:
  bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon

To view output, run:
  bazel run //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon --test_arg=--dump_output

[       OK ] ToolchainFileTest.toolchain/check/testdata/class/fail_todo_import.carbon (64 ms)
```

And here's what a run looks like:

```
$ bazel run //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon --test_arg=--dump_output
(elided bazel output)
Executing tests from //toolchain/testing:file_test
-----------------------------------------------------------------------------
===============================================================================
= toolchain/check/testdata/class/fail_todo_import.carbon
===============================================================================
= stderr
===============================================================================
b.carbon: ERROR: Semantics TODO: `TryResolveImportRefUnused on ClassDecl`.
b.carbon: ERROR: Semantics TODO: `TryResolveImportRefUnused on ClassDecl`.
b.carbon: ERROR: Semantics TODO: `TryResolveInst on ClassType`.
b.carbon: ERROR: Semantics TODO: `TryResolveInst on ClassType`.
b.carbon:18:29: ERROR: Name `d_ref` not found.
var d: (ForwardDeclared,) = d_ref;
                            ^~~~~
===============================================================================
= stdout
===============================================================================
--- a.carbon
(eliding semir output)
}

===============================================================================
= Exit with success: false
===============================================================================

Done!
```

This also switches from PrettyStackTraceFormat to
PrettyStackTraceString. This is partly so that we can share the produced
command (avoiding two different format strings corresponding to the same
value), but also it's in my mind to step away from
llvm/llvm-project#77351.
@jonmeow jonmeow closed this Feb 16, 2024
@jonmeow jonmeow deleted the clang-tidy branch February 16, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PrettyStackTraceFormat prints a null character after the formatted string
2 participants