Is there a reason you want the API to be such that you can get the 'error string', 'allocation string', and metadata? The first two seem nice, though not especially useful without out the stack traces. The third seems like it might be exposing internals we don't want to expose and potentially lead to versioning issues. Also how does the crash reporter already knows the error type when calling these functions? I think perhaps it would make sense to have a getCrashReport() API that fills out a struct and perhaps having dumpReportInternal() only use the information from that struct to avoid duplicating work/ensuring all information we use is exposed? I may not be familiar enough with your crash handler case to understand the justification for why it is structured this way.

Ah, sorry. I should have included a link to a broken build instance. Here's one: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/36121/steps/check-clang%20ubsan/logs/stdio

Fix LGTM.

I noticed in Decl.cpp your change deleted some whitespace that belonged there. Not a big deal, just try to remember to run clang-format-diff when you're submitting for review. Thanks!

Any reason not to switch the other bots as well?

From the revert email http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190722/675580.html they use:
$ gold -v
GNU gold (GNU Binutils for Debian 2.28) 1.14

I'm not very familiar with the compiler-rt build-side of this change, eugenis can speak to that better than I can.

In D63736#1556463, @hctim wrote:

In D63736#1556349, @eugenis wrote:

Do we need similar protection in deallocate?

We shouldn't need to. GPA::deallocate() may call malloc() and free(), but can only be called with guarded allocations. Recursion is okay (and desirable, as the unwinder's implementation is okay to be sampled) in deallocate().

Hi gilang, out of curiosity what is your intended use of the StackSafetyAnalysis? Is it necessary to expose all of the information exposed here?

In D59178#1533262, @mclow.lists wrote:

In D59178#1532032, @vlad.tsyrklevich wrote:

I tested the following change and verified that it works for the numeric example above, but don't have time to write a test and monitor for build failures so I'm going to revert this change for now. Leaving it here for whoever wants to pick it up:

Assuming that I have applied Vlad's patch correctly, I find that it doesn't work.

to_chars(buf, buf + 100, (int64_t)0x10000000000) gives a result of 199511627776 instead of 1099511627776
(note the missing zero)

I tested the following change and verified that it works for the numeric example above, but don't have time to write a test and monitor for build failures so I'm going to revert this change for now. Leaving it here for whoever wants to pick it up:

--- a/libcxx/src/charconv.cpp
+++ b/libcxx/src/charconv.cpp
@@ -176,22 +176,39 @@ __u64toa(uint64_t value, char* buffer)
         const uint32_t b0 = v0 / 10000;
         const uint32_t c0 = v0 % 10000;

After this change landed I started getting odd failures with check-llvm with MSan or ASan like the following: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/12853
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/test/ThinLTO/X86/dot-dumper-full-lto.ll:12:10: error: CHECK: expected string not found in input
; CHECK: subgraph cluster_4294967295
<stdin>:3:2: note: possible intended match here
subgraph cluster_0004294967295 {

LGTM otherwise

One thing I also just realized is that by explicitly setting TLS model to initial-exec we prevent dlopen() dynamic loading, I don't have a problem with that but just wanted to make sure @cryptoad was aware.

In D60593#1483183, @hctim wrote:

• Added a thread contention test. Creates allocations using multiple threads and checks to see that a guarded slot is never allocated twice. Also tests against the random slot selection.

Found some bugs, requesting changes to track that I'd like to review the fixes and tests for that behavior.

LGTM other than nits

I believe the short circuiting of the IsInitialised load/branch in the general case basically leads to the performance being the same. The values are so close that I'm willing to believe that it's within margin-of-error, but some minor microarchitectural feature could also be leading to the ret version being faster.

For the branch case, I had the conditional branch outside of NextSampleCounter == 0, so that the RT-disabled would incur minimal overhead (single branch instead of two branches), e.g.

shouldSample() {
  if (!IsInitialised) return false;
  if (NextSampleCounter == 0`) ...

In D60593#1470991, @hctim wrote:

Option	Median Time (with 20 runs)	Runtime increase relative to CT-Disabled
Compile-time disabled	3.5715s	0.0000%
Runtime disabled (init check in ret)	3.6490s	2.1239%
Runtime disabled (init check in branch)	3.6350s	1.7469%
Runtime enabled (init check in ret)	3.6435s	1.9761%
Runtime enabled (init check in branch)	3.6500s	2.1507%

In D60593#1464932, @hctim wrote:

I've implemented them here because it gives us a few benefits:

1. Allows us to test different configuration options at runtime without a recompile.
2. Gives allocators more choice as to how they use GWP-ASan (they might provide alignment guarantees that we can't).
3. We can A/B test deployments in production. We can have half a fleet using OddLeftEvenRight and half a fleet using OddRightEvenLeft, and this may hypothetically give us the best chance of finding bugs.

Happy to take feedback on this, if others see it as unneccessary we can delete them.

Some thoughts after taking a quick glance at this:

• AllocationStrategy/AlignmentStrategy don’t seem useful to me as they are. Are they just for testing? Could we get rid of them by allocating full-page allocations instead in the under/overflow tests instead? They complicate the implementation and I don’t know why people would use them outside of flipping a flag to say “full right-alignment” instead of whatever reasonable alignment strategy we choose for them. Perhaps collapse both into a single bool PerfectlyRightAlign flag and just have the default alignment strategy be something we choose?
• What is the plan for adding stack trace support?

• Keep x86_64 doc link