Remove the old AddressDescription struct

[filcab]

Summary

Remove the struct since it's used only once. This removes one level of
indirection, and moves all *AddressDescription to be one of the recently
introduced structures.

[vitalybuka]

lib/asan/asan_debugging.cc

82

could you avoid NOLINT with descr = {addr} ?

85

Could you please add:
const char *region_kind = nullptr;
name[0] = 0;

if any bug in switches below, it would be easiest to debug consistent values

86

what is going to happen if descr.kind is not one of your cases?

if it's impossible, CHECK in default: would be nice.

[filcab]

Addressed code review comments

[filcab]

Make sure name and name_size are !=0 and >0 respectively.

[filcab]

This revision got merged with D24131.

lib/asan/asan_debugging.cc

85

I usually avoid initializing when we cover all cases, as it makes it easier for msan to catch the uninitialized use.
But this sanitizer won't be built with MSan, so that defensive coding is a good thing to do. Thanks.

86

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)?

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) {
+  NewAddressDescription descr{addr};  // NOLINT
+  uptr region_address = 0;
+  uptr region_size = 0;
+  const char *region_kind;
+  switch (descr.kind) {
+    case kAddressKindWild:
+      // region_{address,size} are already 0
+      region_kind = "heap-invalid";
+      break;
+    case kAddressKindShadow:
+      // region_{address,size} are already 0
+      switch (descr.shadow.kind) {
+        case kShadowKindLow:
+          region_kind = "low shadow";
+          break;
+        case kShadowKindGap:
+          region_kind = "shadow gap";
+          break;
+        case kShadowKindHigh:
+          region_kind = "high shadow";
+          break;
+      }
+      break;
+    case kAddressKindHeap:
+      region_kind = "heap";
+      name[0] = 0;
+      region_address = descr.heap.chunk_access.chunk_begin;
+      region_size = descr.heap.chunk_access.chunk_size;
+      break;
+    case kAddressKindStack:
+      region_kind = "stack";
+      if (!descr.stack.frame_descr) {
+        name[0] = 0;
+        // region_{address,size} are already 0
+      } else {
+        FindInfoForStackVar(addr, descr.stack.frame_descr, descr.stack.offset,
+                            name, name_size, region_address, region_size);
+      }
+      break;
+    case kAddressKindGlobal:
+      region_kind = "global";
+      auto &g = descr.global.globals[0];
+      internal_strlcpy(name, g.name, name_size);
+      region_address = g.beg;
+      region_size = g.size;
+      break;
+  }
+  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

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);