Add NewAddressDescription, which can describe any type of address.

[filcab]

Summary

This is useful for inclusion in the Error* structures.

The class will be renamed after getting rid of the current AddressDescription,
which is only used in asan_debugging.cc

Split between *Base and the "proper" object with constructors, because VS2013
doesn't allow us to include objects without a trivial default constructor in an
enum, and I'd like to be sure that a NewAddressDescription object is always
valid, so we need initialization.

[vitalybuka]

lib/asan/asan_descriptions.h

176 ↗

(On Diff #70004)

I don't like union like this, but I missed the background for this changes.
What is the ultimate goal?

185 ↗

(On Diff #70004)

Why not regular C++ e.g. with interface with virtual methods?

218 ↗

(On Diff #70004)

Just: *this = {};

[filcab]

Update after code review

[filcab]

Revert back to the old constructor, otherwise we'd get an infinite loop.
Added an extra argument to the non-default constructor, which allows us to control wether the thread registry should be locked or not (pass false when it's already locked).

[vitalybuka]

lib/asan/asan_descriptions.h

185 ↗

(On Diff #70399)

Why not to replace existing ErrorDescription with this one n the same Patch so you don't need to introduce New* names

[vitalybuka]

I like small patches, but they should be meaningful on it's own and incrementally improve code quality.
Here we have patch with "unused" type which is safe be remove by anyone in the next patch.

It would be nice to have smallest possible patch but which has code which explain things like this:

1. Why we need NewAddressDescriptionBase. How this is different from ErrorDescription which is just fine without Base?
2. Why we need shouldLockThreadRegistry?
3. Why not to incrementally change AddressDescription?

lib/asan/asan_descriptions.h

233 ↗

(On Diff #70399)

ThreadRegistryLock

[filcab]

Address review comments. Merge D24132 into this patch.

[filcab]

In D24131#535266, @vitalybuka wrote:

I like small patches, but they should be meaningful on it's own and incrementally improve code quality.
Here we have patch with "unused" type which is safe be remove by anyone in the next patch.

It would be nice to have smallest possible patch but which has code which explain things like this:

1. Why we need NewAddressDescriptionBase. How this is different from ErrorDescription which is just fine without Base?

ErrorDescription is the class that contains the union which VS2013 doesn't like, so it won't need the *Base version since it can initialize itself. The classes *inside* that union can't have non-trivial default initializers in VS2013 because it doesn't support them yet.

2. Why we need shouldLockThreadRegistry?

asan_locate_address doesn't need to lock while running, but if (and only if) the address being described is in the stack, we'll need to lock the thread registry. Since asan_locate_address doesn't even try to print or anything, I think it's a bit too much to always lock, hence the extra constructor argument.

3. Why not to incrementally change AddressDescription?

AddressDescription is too basic and not very useful. The whole point of the {Stack,Global,Shadow,Heap}AddressDescription structures is to have a good amount of information about the address. Their purpose is to be serialized so we can properly support post-mortem debugging without the debugger needing to know about all the sanitizer internals and reproduce that work.

Since we already have all that information, but in different structs, we can replace the old AddressDescription with a more descriptive structure, which can contain any of the other descriptions, depending on the address.

lib/asan/asan_descriptions.h

218 ↗

(On Diff #70399)

*this = {} will trigger an infinite loop, since it will call itself.

185 ↗

(On Diff #70004)

I would like to be able to use the same structures internally and externally, when reporting errors.
Having a trivially copiable type makes it much easier. Not having to deal with the tagged union and having virtual methods would make the type non-trivially copiable and then we would have to add a bunch of code for (de)serializing the type so the debugger can get to it.

The rationale for the structured error reporting is here:
http://lists.llvm.org/pipermail/llvm-dev/2016-July/101933.html

[filcab]

Bring over a comment from D24132 that wasn't submitted.

lib/asan/asan_debugging.cc

87 ↗

(On Diff #70530)

If we add the default case, we get a clang warning, though. I can add a check that we have one of those, before the switch, but it will end up ugly:

CHECK(descr.kind == kAddressKindWild || descr.kind == kAddressKindShadow || ...);
Should we also fix places like PrintHeapChunkAccess in asan_descriptions.cc, which does the same thing (not have a default case)?

[vitalybuka]

lib/asan/asan_debugging.cc

87 ↗

(On Diff #70530)

We have another internal switch so assert(region_kind) just before return should cover both

108 ↗

(On Diff #70530)

name[0] = 0; is already done at entry

115 ↗

(On Diff #70530)

name[0] = 0; is already done at entry

lib/asan/asan_descriptions.h

185 ↗

(On Diff #70530)

I see.
How about following?

struct AddressDescriptionData {
 AddressKind kind;
 union {
    ShadowAddressDescription shadow;
    HeapAddressDescription heap;
    StackAddressDescription stack;
    GlobalAddressDescription global;
    uptr addr;
  };
  // No methods.
}

class AddressDescription {
 private:
  AddressDescriptionData data;

 public:
  AddressDescription() {
    ...
  }
   uptr Address() const ;
   uptr Print() const ;
   const ShadowAddressDescription* AsShadowAddress() const { 
      return "kind is ShadowAddress" ? &shadow : nullptr;
   };
}

[filcab]

Addressed some code review comments.

lib/asan/asan_descriptions.h

185 ↗

(On Diff #70530)

But then you'd either be copying AddressDescriptionData a bunch, to create new AddressDescription objects with the methods you want, or you'd end up with something like AddressDescriptionData, AddressDescription, and AddressDescriptionView, which would allow you to:

• Store the info wherever you want (AddressDescriptionData)
• Get the info from and address easily (AddressDescription. Owns the data)
• "Interpret" an AddressDescriptionData (AddressDescriptionView, contains a reference)

This would end up duplicating the methods (AddressDescription's methods can be defined in terms of AddressDescriptionView's, but it would still be duplicating work.
I'm also not sure what we'd gain by not having the utility methods in the base object. I see the argument of disallowing property changes on AddressDescription and its private member, but we only use a full object like that once: in __asan_locate_address. Most of the other uses for this structure will be in the Error* structs. In there we'd have the fully public class, so we wouldn't be able to protect against inadvertent writes.

We can also do without AddressDescription, and just have the Data+View (not necessarily those names, of course). This would make __asan_locate_address slightly more complicated, since it would need to create a *Data object, and a *View object (probably the *View would populate the data).
I don't see that as a big win anywhere, but I'm ok with doing it if you prefer.

[vitalybuka]

lib/asan/asan_descriptions.h

185 ↗

(On Diff #70694)

I like this way: https://reviews.llvm.org/D24358
WDYT?

[filcab]

Updated with review comments.
I wasn't looking at the class properly, it fits in an Error* structure and the type is still trivially-copyable.

Vitaly: I changed the patch a bit from yours. I think it ends up being simpler to have code like:

if (auto shadow = descr.AsShadow()) {
  ...
} else ...

Than to have the switch on .Kind(), since there's more code (reading) overhead with the switch.

I kept the const you added to the methods like Print(), but I can take them out and do a follow-up patch (or do it before) if you'd prefer.

Thank you,

Filipe

[filcab]

Make it work on VS2013: only trivial initialization.
Added UNREACHABLE to the end of covered switches (silences VS warnings).

[vitalybuka]

LGTM with few comments

In D24131#538393, @filcab wrote:

Vitaly: I changed the patch a bit from yours. I think it ends up being simpler to have code like:

if (auto shadow = descr.AsShadow()) {
  ...
} else ...

LGTM

Than to have the switch on .Kind(), since there's more code (reading) overhead with the switch.

I kept the const you added to the methods like Print(), but I can take them out and do a follow-up patch (or do it before) if you'd prefer.

Thanks

lib/asan/asan_descriptions.h

198 ↗

(On Diff #70855)

Could you please hide this constructor into cpp?
also GetHeapAddressInformation and DescribeAddressIfStack can be removed from h.

227 ↗

(On Diff #70855)

Please remove commented line

[vitalybuka]

Almost forgot, it could be useful if you copy/paste your proposal into new PR assigned to yourself, and include that PR into all patches. Can help future readers with context.

[vitalybuka]

lib/asan/asan_descriptions.h

197 ↗

(On Diff #70855)

And I'd prefer for now remove shouldLockThreadRegistry argument and do ThreadRegistryLock inside of GetStackAddressInformation uconditionally

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

 #include "asan_internal.h"
 #include "asan_mapping.h"
 #include "asan_report.h"
 #include "asan_thread.h"
 
 namespace {
 using namespace __asan;
 
-void FindInfoForStackVar(uptr addr, const char *frame_descr, uptr offset,
-                         AddressDescription *descr) {
+static void FindInfoForStackVar(uptr addr, const char *frame_descr, uptr offset,
+                                char *name, uptr name_size,
+                                uptr &region_address, uptr &region_size) {
   InternalMmapVector<StackVarDescr> vars(16);
   if (!ParseFrameDescription(frame_descr, &vars)) {
     return;
   }
 
   for (uptr i = 0; i < vars.size(); i++) {
     if (offset <= vars[i].beg + vars[i].size) {
       // We use name_len + 1 because strlcpy will guarantee a \0 at the end, so
       // if we're limiting the copy due to name_len, we add 1 to ensure we copy
       // the whole name and then terminate with '\0'.
-      internal_strlcpy(descr->name, vars[i].name_pos,
-                       Min(descr->name_size, vars[i].name_len + 1));
-      descr->region_address = addr - (offset - vars[i].beg);
-      descr->region_size = vars[i].size;
+      internal_strlcpy(name, vars[i].name_pos,
+                       Min(name_size, vars[i].name_len + 1));
+      region_address = addr - (offset - vars[i].beg);
+      region_size = vars[i].size;
       return;
     }
   }
 }
 
-void AsanLocateAddress(uptr addr, AddressDescription *descr) {
-  ShadowAddressDescription shadow_descr;
-  if (GetShadowAddressInformation(addr, &shadow_descr)) {
-    descr->region_kind = ShadowNames[shadow_descr.kind];
-    return;
-  }
-  GlobalAddressDescription global_descr;
-  if (GetGlobalAddressInformation(addr, &global_descr)) {
-    descr->region_kind = "global";
-    auto &g = global_descr.globals[0];
-    internal_strlcpy(descr->name, g.name, descr->name_size);
-    descr->region_address = g.beg;
-    descr->region_size = g.size;
-    return;
-  }
-
-  StackAddressDescription stack_descr;
-  asanThreadRegistry().Lock();
-  if (GetStackAddressInformation(addr, &stack_descr)) {
-    asanThreadRegistry().Unlock();
-    descr->region_kind = "stack";
-    if (!stack_descr.frame_descr) {
-      descr->name[0] = 0;
-      descr->region_address = 0;
-      descr->region_size = 0;
-    } else {
-      FindInfoForStackVar(addr, stack_descr.frame_descr, stack_descr.offset,
-                          descr);
-    }
-    return;
-  }
-  asanThreadRegistry().Unlock();
-
-  descr->name[0] = 0;
-  HeapAddressDescription heap_descr;
-  if (GetHeapAddressInformation(addr, 1, &heap_descr)) {
-    descr->region_address = heap_descr.chunk_access.chunk_begin;
-    descr->region_size = heap_descr.chunk_access.chunk_size;
-    descr->region_kind = "heap";
-    return;
-  }
-
-  descr->region_address = 0;
-  descr->region_size = 0;
-  descr->region_kind = "heap-invalid";
-}
-
 uptr AsanGetStack(uptr addr, uptr *trace, u32 size, u32 *thread_id,
                          bool alloc_stack) {
   AsanChunkView chunk = FindHeapChunkByAddress(addr);
   if (!chunk.IsValid()) return 0;
 
   StackTrace stack(nullptr, 0);
   if (alloc_stack) {
     if (chunk.AllocTid() == kInvalidTid) return 0;
uptr AsanGetStack(uptr addr, uptr *trace, u32 size, u32 *thread_id,
 
   return 0;
 }
 
 }  // namespace
 
 SANITIZER_INTERFACE_ATTRIBUTE
 const char *__asan_locate_address(uptr addr, char *name, uptr name_size,
-                                  uptr *region_address, uptr *region_size) {
-  AddressDescription descr = { name, name_size, 0, 0, nullptr };
-  AsanLocateAddress(addr, &descr);
-  if (region_address) *region_address = descr.region_address;
-  if (region_size) *region_size = descr.region_size;
-  return descr.region_kind;
+                                  uptr *region_address_ptr,
+                                  uptr *region_size_ptr) {
+  AddressDescription descr(addr);
+  uptr region_address = 0;
+  uptr region_size = 0;
+  const char *region_kind = nullptr;
+  if (name && name_size > 0) name[0] = 0;
+
+  if (auto shadow = descr.AsShadow()) {
+    // region_{address,size} are already 0
+    switch (shadow->kind) {
+      case kShadowKindLow:
+        region_kind = "low shadow";
+        break;
+      case kShadowKindGap:
+        region_kind = "shadow gap";
+        break;
+      case kShadowKindHigh:
+        region_kind = "high shadow";
+        break;
+    }
+  } else if (auto heap = descr.AsHeap()) {
+    region_kind = "heap";
+    region_address = heap->chunk_access.chunk_begin;
+    region_size = heap->chunk_access.chunk_size;
+  } else if (auto stack = descr.AsStack()) {
+    region_kind = "stack";
+    if (!stack->frame_descr) {
+      // region_{address,size} are already 0
+    } else {
+      FindInfoForStackVar(addr, stack->frame_descr, stack->offset, name,
+                          name_size, region_address, region_size);
+    }
+  } else if (auto global = descr.AsGlobal()) {
+    region_kind = "global";
+    auto &g = global->globals[0];
+    internal_strlcpy(name, g.name, name_size);
+    region_address = g.beg;
+    region_size = g.size;
+  } else {
+    // region_{address,size} are already 0
+    region_kind = "heap-invalid";
+  }
+
+  CHECK(region_kind);
+  if (region_address_ptr) *region_address_ptr = region_address;
+  if (region_size_ptr) *region_size_ptr = region_size;
+  return region_kind;
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 uptr __asan_get_alloc_stack(uptr addr, uptr *trace, uptr size, u32 *thread_id) {
   return AsanGetStack(addr, trace, size, thread_id, /* alloc_stack */ true);
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE

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

 static const char *const ShadowNames[] = {"low shadow", "shadow gap",
                                           "high shadow"};
 
 struct ShadowAddressDescription {
   uptr addr;
   ShadowKind kind;
   u8 shadow_byte;
 
-  void Print();
+  void Print() const;
 };
 
 bool GetShadowAddressInformation(uptr addr, ShadowAddressDescription *descr);
 bool DescribeAddressIfShadow(uptr addr);
 
 enum AccessType {
   kAccessTypeLeft,
   kAccessTypeRight,

 struct HeapAddressDescription {
   uptr addr;
   uptr alloc_tid;
   uptr free_tid;
   u32 alloc_stack_id;
   u32 free_stack_id;
   ChunkAccess chunk_access;
 
-  void Print();
+  void Print() const;
 };
 
 bool GetHeapAddressInformation(uptr addr, uptr access_size,
                                HeapAddressDescription *descr);
 bool DescribeAddressIfHeap(uptr addr, uptr access_size = 1);
 
 struct StackAddressDescription {
   uptr addr;
   uptr tid;
   uptr offset;
   uptr frame_pc;
   const char *frame_descr;
 
-  void Print(uptr access_size = 1);
+  void Print(uptr access_size = 1) const;
 };
 
 bool GetStackAddressInformation(uptr addr, StackAddressDescription *descr);
-bool DescribeAddressIfStack(uptr addr, uptr access_size);
 
 struct GlobalAddressDescription {
   uptr addr;
   // Assume address is close to at most four globals.
   static const int kMaxGlobals = 4;
   __asan_global globals[kMaxGlobals];
   u32 reg_sites[kMaxGlobals];
   u8 size;
 
-  void Print(uptr access_size = 1, const char *bug_type = "");
+  void Print(uptr access_size = 1, const char *bug_type = "") const;
 };
 
 bool GetGlobalAddressInformation(uptr addr, GlobalAddressDescription *descr);
 bool DescribeAddressIfGlobal(uptr addr, uptr access_size, const char *bug_type);
 
 // General function to describe an address. Will try to describe the address as
 // a shadow, global (variable), stack, or heap address.
 // bug_type is optional and is used for checking if we're reporting an
 // initialization-order-fiasco
 // The proper access_size should be passed for stack, global, and heap
 // addresses. Defaults to 1.
 // Each of the *AddressDescription functions has its own Print() member, which
 // may take access_size and bug_type parameters if needed.
 void PrintAddressDescription(uptr addr, uptr access_size = 1,
                              const char *bug_type = "");
 
+enum AddressKind {
+  kAddressKindWild,
+  kAddressKindShadow,
+  kAddressKindHeap,
+  kAddressKindStack,
+  kAddressKindGlobal,
+};
+
+class AddressDescription {
+  struct AddressDescriptionData {
+    AddressKind kind;
+    union {
+      ShadowAddressDescription shadow;
+      HeapAddressDescription heap;
+      StackAddressDescription stack;
+      GlobalAddressDescription global;
+      uptr addr;
+    };
+  };
+
+  AddressDescriptionData data;
+
+ public:
+  AddressDescription() = default;
+  // shouldLockThreadRegistry allows us to skip locking if we're sure we already
+  // have done it.
+  AddressDescription(uptr addr, bool shouldLockThreadRegistry = true)
+      : AddressDescription(addr, 1, shouldLockThreadRegistry) {}
+  AddressDescription(uptr addr, uptr access_size,
+                     bool shouldLockThreadRegistry = true);
+
+  uptr Address() const {
+    switch (data.kind) {
+      case kAddressKindWild:
+        return data.addr;
+      case kAddressKindShadow:
+        return data.shadow.addr;
+      case kAddressKindHeap:
+        return data.heap.addr;
+      case kAddressKindStack:
+        return data.stack.addr;
+      case kAddressKindGlobal:
+        return data.global.addr;
+    }
+    UNREACHABLE("AddressInformation kind is invalid");
+  }
+  void Print() const {
+    switch (data.kind) {
+      case kAddressKindWild:
+        Printf("Address %p is a wild pointer.\n", data.addr);
+        return;
+      case kAddressKindShadow:
+        return data.shadow.Print();
+      case kAddressKindHeap:
+        return data.heap.Print();
+      case kAddressKindStack:
+        return data.stack.Print();
+      case kAddressKindGlobal:
+        return data.global.Print();
+    }
+    UNREACHABLE("AddressInformation kind is invalid");
+  }
+
+  void StoreTo(AddressDescriptionData *dst) const { *dst = data; }
+
+  const ShadowAddressDescription *AsShadow() const {
+    return data.kind == kAddressKindShadow ? &data.shadow : nullptr;
+  }
+  const HeapAddressDescription *AsHeap() const {
+    return data.kind == kAddressKindHeap ? &data.heap : nullptr;
+  }
+  const StackAddressDescription *AsStack() const {
+    return data.kind == kAddressKindStack ? &data.stack : nullptr;
+  }
+  const GlobalAddressDescription *AsGlobal() const {
+    return data.kind == kAddressKindGlobal ? &data.global : nullptr;
+  }
+};
+
 }  // namespace __asan
 
 #endif  // ASAN_DESCRIPTIONS_H

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

bool DescribeAddressIfGlobal(uptr addr, uptr access_size,
                              const char *bug_type) {
   GlobalAddressDescription descr;
   if (!GetGlobalAddressInformation(addr, &descr)) return false;
 
   descr.Print(access_size, bug_type);
   return true;
 }
 
-void ShadowAddressDescription::Print() {
+void ShadowAddressDescription::Print() const {
   Printf("Address %p is located in the %s area.\n", addr, ShadowNames[kind]);
 }
 
-void GlobalAddressDescription::Print(uptr access_size, const char *bug_type) {
+void GlobalAddressDescription::Print(uptr access_size,
+                                     const char *bug_type) const {
   for (int i = 0; i < size; i++) {
     DescribeAddressRelativeToGlobal(addr, access_size, globals[i]);
     if (0 == internal_strcmp(bug_type, "initialization-order-fiasco") &&
         reg_sites[i]) {
       Printf("  registered at:\n");
       StackDepotGet(reg_sites[i]).Print();
     }
   }
 }
 
-void StackAddressDescription::Print(uptr access_size) {
+void StackAddressDescription::Print(uptr access_size) const {
   Decorator d;
   char tname[128];
   Printf("%s", d.Location());
   Printf("Address %p is located in stack of thread T%d%s", addr, tid,
          ThreadNameWithParenthesis(tid, tname, sizeof(tname)));
 
   if (!frame_descr) {
     Printf("%s\n", d.EndLocation());
void StackAddressDescription::Print(uptr access_size) const {
   if (SANITIZER_WINDOWS)
     Printf("      (longjmp, SEH and C++ exceptions *are* supported)\n");
   else
     Printf("      (longjmp and C++ exceptions *are* supported)\n");
 
   DescribeThread(GetThreadContextByTidLocked(tid));
 }
 
-void HeapAddressDescription::Print() {
+void HeapAddressDescription::Print() const {
   PrintHeapChunkAccess(addr, chunk_access);
 
   asanThreadRegistry().CheckLocked();
   AsanThreadContext *alloc_thread = GetThreadContextByTidLocked(alloc_tid);
   StackTrace alloc_stack = GetStackTraceFromId(alloc_stack_id);
 
   char tname[128];
   Decorator d;
Printf("%sallocated by thread T%d%s here:%s\n", d.Allocation(),
            d.EndAllocation());
   }
   alloc_stack.Print();
   DescribeThread(GetCurrentThread());
   if (free_thread) DescribeThread(free_thread);
   DescribeThread(alloc_thread);
 }
 
+AddressDescription::AddressDescription(uptr addr, uptr access_size,
+                                       bool shouldLockThreadRegistry) {
+  if (GetShadowAddressInformation(addr, &data.shadow)) {
+    data.kind = kAddressKindShadow;
+    return;
+  }
+  if (GetHeapAddressInformation(addr, access_size, &data.heap)) {
+    data.kind = kAddressKindHeap;
+    return;
+  }
+
+  bool isStackMemory = false;
+  if (shouldLockThreadRegistry) {
+    ThreadRegistryLock l(&asanThreadRegistry());
+    isStackMemory = GetStackAddressInformation(addr, &data.stack);
+  } else {
+    isStackMemory = GetStackAddressInformation(addr, &data.stack);
+  }
+  if (isStackMemory) {
+    data.kind = kAddressKindStack;
+    return;
+  }
+
+  if (GetGlobalAddressInformation(addr, &data.global)) {
+    data.kind = kAddressKindGlobal;
+    return;
+  }
+  data.kind = kAddressKindWild;
+  addr = 0;
+}
+
 void PrintAddressDescription(uptr addr, uptr access_size,
                              const char *bug_type) {
   ShadowAddressDescription shadow_descr;
   if (GetShadowAddressInformation(addr, &shadow_descr)) {
     shadow_descr.Print();
     return;
   }
 

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

 
 struct StackVarDescr {
   uptr beg;
   uptr size;
   const char *name_pos;
   uptr name_len;
 };
 
-struct AddressDescription {
-  char *name;
-  uptr name_size;
-  uptr region_address;
-  uptr region_size;
-  const char *region_kind;
-};
-
 // Returns the number of globals close to the provided address and copies
 // them to "globals" array.
 int GetGlobalsForAddress(uptr addr, __asan_global *globals, u32 *reg_sites,
                          int max_globals);
 
 const char *MaybeDemangleGlobalName(const char *name);
 void PrintGlobalNameIfASCII(InternalScopedString *str, const __asan_global &g);
 void PrintGlobalLocation(InternalScopedString *str, const __asan_global &g);