[PATCH] D24131: Add NewAddressDescription, which can describe any type of address.

[Filipe Cabecinhas via llvm-commits]

filcab added inline comments.

================
Comment at: lib/asan/asan_descriptions.h:185
@@ +184,3 @@
+  // We should be able to do without this struct after we deprecate VS2013.
+  AddressKind kind;
+  union {
----------------
vitalybuka wrote:
> 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;
>    };
> }
> 
> 
> ```
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.


https://reviews.llvm.org/D24131