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

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

185

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

218

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

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

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

185

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

218

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

[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

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

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

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

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

227

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

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

lib/asan/asan_descriptions.h

 // 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,
+};
+
+struct NewAddressDescriptionBase {
+  AddressKind kind;
+  union {
+    ShadowAddressDescription shadow;
+    HeapAddressDescription heap;
+    StackAddressDescription stack;
+    GlobalAddressDescription global;
+    uptr addr;
+  };
+  uptr Address() {
+    switch (kind) {
+      case kAddressKindWild:
+        return addr;
+      case kAddressKindShadow:
+        return shadow.addr;
+      case kAddressKindHeap:
+        return heap.addr;
+      case kAddressKindStack:
+        return stack.addr;
+      case kAddressKindGlobal:
+        return global.addr;
+    }
+  }
+  void Print() {
+    switch (kind) {
+      case kAddressKindWild:
+        Printf("Address %p is a wild pointer.\n", addr);
+        return;
+      case kAddressKindShadow:
+        return shadow.Print();
+      case kAddressKindHeap:
+        return heap.Print();
+      case kAddressKindStack:
+        return stack.Print();
+      case kAddressKindGlobal:
+        return global.Print();
+    }
+  }
+};
+
+struct NewAddressDescription : NewAddressDescriptionBase {
+  NewAddressDescription() {
+    kind = kAddressKindWild;
+    addr = 0;
+  }
+  explicit NewAddressDescription(uptr addr) {
+    if (GetShadowAddressInformation(addr, &shadow)) {
+      kind = kAddressKindShadow;
+      return;
+    }
+    if (GetHeapAddressInformation(addr, 1, &heap)) {
+      kind = kAddressKindHeap;
+      return;
+    }
+    asanThreadRegistry().Lock();
+    if (GetStackAddressInformation(addr, &stack)) {
+      kind = kAddressKindStack;
+      asanThreadRegistry().Unlock();
+      return;
+    }
+    asanThreadRegistry().Unlock();
+    if (GetGlobalAddressInformation(addr, &global)) {
+      kind = kAddressKindGlobal;
+      return;
+    }
+    kind = kAddressKindWild;
+    addr = 0;
+  }
+};
+
 }  // namespace __asan
 
 #endif  // ASAN_DESCRIPTIONS_H