[PATCH] D74324: Tools emit the bug report URL on crash

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Support/PrettyStackTrace.cpp:145
 
+static const char* customBugReportMsg;
+
----------------
jhenderson wrote:
> jhenderson wrote:
> > `static const char *BugReportMsg;`
> > 
> > Why not just have this set to the default in the first place, and overwrite it if somebody needs to? That'll remove the need for the new `if` in the `CrashHandler` function.
> Again, this hasn't been properly addresssed. This should use an upper-case letter for the first letter of variable names. Please see [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | the coding standards ]].
`static const char BugReportMsg[] = ...`

`static const char *BugReportMsg` defines a writable variable.

In IR, the former does not have the unnamed_addr attribute while the latter has. The latter can lower to .rodata.str1.1 (SHF_MERGE | SHF_STRINGS) which can be optimized by the linker. This is nuance and the optimization probably weighs nothing (it is hardly to think there will be another thing having the same byte sequence.) The former just seems more correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74324/new/

https://reviews.llvm.org/D74324