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

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

In https://reviews.llvm.org/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.


================
Comment at: lib/asan/asan_descriptions.h:185
@@ +184,3 @@
+  };
+  uptr Address() {
+    switch (kind) {
----------------
vitalybuka wrote:
> Why not regular C++ e.g. with interface with virtual methods?
> 
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

================
Comment at: lib/asan/asan_descriptions.h:218
@@ +217,3 @@
+  NewAddressDescription() {
+    kind = kAddressKindWild;
+    addr = 0;
----------------
`*this = {}` will trigger an infinite loop, since it will call itself.


https://reviews.llvm.org/D24131