Start reifying error descriptions

[filcab]

Summary

This commit sets up the infrastructure to use reified error
descriptions, and moves ReportStackOverflow to the new system.

After we convert all the errors, we'll be able to simplify ScopedInErrorReport
and remove the older debugging mechanism which had some errors partly reified
in some way. We'll be able to maintain the external API.

ScopedInErrorReport will be able to track one of the reified errors at a time.
The purpose of this is so we have its destructor actually print the error and
possibly interface with the debugger (will depend on the platform, of course).

[filcab]

Use member initializers;
Remove dead code per Timothy Wood's review (off-list).

[kcc]

lib/asan/asan_errors.h

51 ↗

(On Diff #68697)

Errr. This looks like a part of change...
What's going to be next?
And why enum? Can we use C++ here (inheritance)?

[filcab]

lib/asan/asan_errors.h

51 ↗

(On Diff #68697)

D23717 has an example of an addition to this. The next errors will add to that union, I tried minimizing the change, while keeping what we'll have later, like that union.

I'd rather avoid inheritance + virtual functions since that makes the type non-trivially copiable. I would like to have the same structures internally + externally. This way, the debugger can simply copy over the current_error_ on ScopedInErrorReport and not need to call any functions on the target. This also allows for easy embedding in core dumps.

We *can* have the virtual functions + etc internally, and then have ASan convert to an external representation, but I don't think it's worth it. It would be a lot more code than a tagged union like we have.

[filcab]

Add NOLINT and clang-format

[filcab]

Use a const ref

[filcab]

D23875 will help dealing with the tagged enum.

[kcc]

lib/asan/asan_errors.h

52 ↗

(On Diff #69234)

At the very least -- move this paragraph into a comment.

lib/asan/asan_report.cc

311 ↗

(On Diff #69234)

use CHECK_EQ

[filcab]

Addressed review comments

[kcc]

lib/asan/asan_errors.h

50 ↗

(On Diff #69351)

Sorry, I did not mean to literally copy the discussion into the comments.
The comment should not be in a form of discussion, but in a form of statement.

// We do X because Y and Z

[filcab]

Rephrase comment.

lib/asan/asan_errors.h

50 ↗

(On Diff #69351)

Wow, that was especially lazy. I've rewritten the comment now. I think it contains the main points.

[kcc]

LGTM with a nit.
Please ping the next change that you want me to review

lib/asan/asan_errors.h

50 ↗

(On Diff #69398)

don't include the user name here (that's not a TODO)

compiler-rt/trunk/lib/asan/CMakeLists.txt

 # Build for the AddressSanitizer runtime support library.
 
 set(ASAN_SOURCES
   asan_allocator.cc
   asan_activation.cc
   asan_debugging.cc
   asan_descriptions.cc
+  asan_errors.cc
   asan_fake_stack.cc
   asan_flags.cc
   asan_globals.cc
   asan_interceptors.cc
   asan_linux.cc
   asan_mac.cc
   asan_malloc_linux.cc
   asan_malloc_mac.cc

compiler-rt/trunk/lib/asan/asan_errors.h

+//===-- asan_errors.h -------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// ASan-private header for error structures.
+//===----------------------------------------------------------------------===//
+#ifndef ASAN_ERRORS_H
+#define ASAN_ERRORS_H
+
+#include "asan_descriptions.h"
+#include "asan_scariness_score.h"
+
+namespace __asan {
+
+struct ErrorBase {
+  ScarinessScore scariness;
+};
+
+struct ErrorStackOverflow : ErrorBase {
+  u32 tid;
+  uptr addr, pc, bp, sp;
+  // ErrorStackOverflow never owns the context.
+  void *context;
+  ErrorStackOverflow(const SignalContext &sig, u32 tid_)
+      : tid(tid_),
+        addr(sig.addr),
+        pc(sig.pc),
+        bp(sig.bp),
+        sp(sig.sp),
+        context(sig.context) {
+    scariness.Scare(10, "stack-overflow");
+  }
+  void Print();
+};
+
+enum ErrorKind {
+  kErrorKindInvalid = 0,
+  kErrorKindStackOverflow,
+};
+
+struct ErrorDescription {
+  ErrorKind kind;
+  // We're using a tagged union because it allows us to have a trivially
+  // copiable type and use the same structures as the public interface.
+  //
+  // We can add a wrapper around it to make it "more c++-like", but that would
+  // add a lot of code and the benefit wouldn't be that big.
+  union {
+    ErrorStackOverflow stack_overflow;
+  };
+  ErrorDescription() { internal_memset(this, 0, sizeof(*this)); }
+  ErrorDescription(const ErrorStackOverflow &e)  // NOLINT
+      : kind(kErrorKindStackOverflow),
+        stack_overflow(e) {}
+
+  bool IsValid() { return kind != kErrorKindInvalid; }
+  void Print() {
+    switch (kind) {
+      case kErrorKindStackOverflow:
+        stack_overflow.Print();
+        return;
+      case kErrorKindInvalid:
+        CHECK(0);
+    }
+    CHECK(0);
+  }
+};
+
+}  // namespace __asan
+
+#endif  // ASAN_ERRORS_H

compiler-rt/trunk/lib/asan/asan_errors.cc

+//===-- asan_errors.cc ------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// ASan implementation for error structures.
+//===----------------------------------------------------------------------===//
+
+#include "asan_errors.h"
+#include "asan_stack.h"
+
+namespace __asan {
+
+void ErrorStackOverflow::Print() {
+  Decorator d;
+  Printf("%s", d.Warning());
+  Report(
+      "ERROR: AddressSanitizer: stack-overflow on address %p"
+      " (pc %p bp %p sp %p T%d)\n",
+      (void *)addr, (void *)pc, (void *)bp, (void *)sp, tid);
+  Printf("%s", d.EndWarning());
+  scariness.Print();
+  BufferedStackTrace stack;
+  GetStackTraceWithPcBpAndContext(&stack, kStackTraceMax, pc, bp, context,
+                                  common_flags()->fast_unwind_on_fatal);
+  stack.Print();
+  ReportErrorSummary("stack-overflow", &stack);
+}
+
+}  // namespace __asan

compiler-rt/trunk/lib/asan/asan_report.cc

 //===-- asan_report.cc ----------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of AddressSanitizer, an address sanity checker.
 //
 // This file contains error reporting code.
 //===----------------------------------------------------------------------===//
 
+#include "asan_errors.h"
 #include "asan_flags.h"
 #include "asan_descriptions.h"
 #include "asan_internal.h"
 #include "asan_mapping.h"
 #include "asan_report.h"
 #include "asan_scariness_score.h"
 #include "asan_stack.h"
 #include "asan_thread.h"
if (halt_on_error_) {
       // so it's safe to wait
       lock_.Lock();
     }
 
     StartReporting(report);
   }
 
   ~ScopedInErrorReport() {
+    if (current_error_.IsValid()) current_error_.Print();
+
     // Make sure the current thread is announced.
     DescribeThread(GetCurrentThread());
     // We may want to grab this lock again when printing stats.
     asanThreadRegistry().Unlock();
     // Print memory stats.
     if (flags()->print_stats)
       __asan_print_accumulated_stats();
 
~ScopedInErrorReport() {
     reporting_thread_tid_ = kInvalidTid;
     lock_.Unlock();
     if (halt_on_error_) {
       Report("ABORTING\n");
       Die();
     }
   }
 
+  void ReportError(const ErrorDescription &description) {
+    // Can only report one error per ScopedInErrorReport.
+    CHECK_EQ(current_error_.kind, kErrorKindInvalid);
+    current_error_ = description;
+  }
+
  private:
   void StartReporting(ReportData *report) {
     if (report) report_data = *report;
     report_happened = true;
     ASAN_ON_ERROR();
     // Make sure the registry and sanitizer report mutexes are locked while
     // we're printing an error report.
     // We can lock them only here to avoid self-deadlock in case of
     // recursive reports.
     asanThreadRegistry().Lock();
     CommonSanitizerReportMutex.Lock();
     reporting_thread_tid_ = GetCurrentTidOrInvalid();
     Printf("===================================================="
            "=============\n");
   }
 
   static StaticSpinMutex lock_;
   static u32 reporting_thread_tid_;
+  // Error currently being reported. This enables the destructor to interact
+  // with the debugger and point it to an error description.
+  static ErrorDescription current_error_;
   bool halt_on_error_;
 };
 
 StaticSpinMutex ScopedInErrorReport::lock_;
 u32 ScopedInErrorReport::reporting_thread_tid_ = kInvalidTid;
+ErrorDescription ScopedInErrorReport::current_error_;
 
 void ReportStackOverflow(const SignalContext &sig) {
   ScopedInErrorReport in_report(/*report*/ nullptr, /*fatal*/ true);
-  Decorator d;
-  Printf("%s", d.Warning());
-  Report(
-      "ERROR: AddressSanitizer: stack-overflow on address %p"
-      " (pc %p bp %p sp %p T%d)\n",
-      (void *)sig.addr, (void *)sig.pc, (void *)sig.bp, (void *)sig.sp,
-      GetCurrentTidOrInvalid());
-  Printf("%s", d.EndWarning());
-  ScarinessScore::PrintSimple(10, "stack-overflow");
-  GET_STACK_TRACE_SIGNAL(sig);
-  stack.Print();
-  ReportErrorSummary("stack-overflow", &stack);
+  ErrorStackOverflow error{sig, GetCurrentTidOrInvalid()};  // NOLINT
+  in_report.ReportError(error);
 }
 
 void ReportDeadlySignal(const char *description, const SignalContext &sig) {
   ScopedInErrorReport in_report(/*report*/ nullptr, /*fatal*/ true);
   Decorator d;
   Printf("%s", d.Warning());
   Report(
       "ERROR: AddressSanitizer: %s on unknown address %p"