On 24/07/2019 08:33, Bert wrote:
> On Tuesday, 23 July 2019 at 20:57:17 UTC, Rainer Schuetze wrote
>> As it turned out, the problem was (again?) a false entry in the class
>> name cache that was added when an invalid pointer (including null) was
>> evaluated (and its dynamic was tried to be determined). Stupid bug, but
>> that happens.
>>
>> You can try the fixed version of MagoNatCC.dll from https://ci.appveyor.com/project/rainers/mago/build/artifacts
> 
> That worked! I still find it very odd that it would exhibit it with irrelevant lines of code removed. Was that changing the cached version or something? Remember I basically have several Q's in my program all virtually identical and some would work and others wouldn't.
> 
> Could you post some of the code that you fixed(or I guess push it to master) when you get a chance... I'm just curious to what it looks like. While such bugs are very annoying and better coding methods should be used, I imagine that this type of scenario for interface resolution is probably well localized and this would probably be the last bug fix for it? If not maybe some logging could be implemented. Interfaces should always be resolvable so if the code fails it is a bug in Visual D and having it logged in some way might help future cases.

The bugfix commit is https://github.com/rainers/mago/commit/9801da089a4b4e9ffa8f1442f4b0aff5852d5fa9?w=1.

The problem was that evaluating the dynamic type of any uninitialized or null class/interface pointer could overwrite a previously evaluated entry in the vtbl->class name cache depending on what happens to be in an uninitialized local variable.

Hopefully the last bug in that area, but you never know. Adding logging would help diagnosing bugs but would have to be more global.

On Wednesday, 24 July 2019 at 15:32:01 UTC, Rainer Schuetze wrote:
>
>
> On 24/07/2019 08:33, Bert wrote:
>> On Tuesday, 23 July 2019 at 20:57:17 UTC, Rainer Schuetze wrote
>>> As it turned out, the problem was (again?) a false entry in the class
>>> name cache that was added when an invalid pointer (including null) was
>>> evaluated (and its dynamic was tried to be determined). Stupid bug, but
>>> that happens.
>>>
>>> You can try the fixed version of MagoNatCC.dll from https://ci.appveyor.com/project/rainers/mago/build/artifacts
>> 
>> That worked! I still find it very odd that it would exhibit it with irrelevant lines of code removed. Was that changing the cached version or something? Remember I basically have several Q's in my program all virtually identical and some would work and others wouldn't.
>> 
>> Could you post some of the code that you fixed(or I guess push it to master) when you get a chance... I'm just curious to what it looks like. While such bugs are very annoying and better coding methods should be used, I imagine that this type of scenario for interface resolution is probably well localized and this would probably be the last bug fix for it? If not maybe some logging could be implemented. Interfaces should always be resolvable so if the code fails it is a bug in Visual D and having it logged in some way might help future cases.
>
> The bugfix commit is https://github.com/rainers/mago/commit/9801da089a4b4e9ffa8f1442f4b0aff5852d5fa9?w=1.
>
> The problem was that evaluating the dynamic type of any uninitialized or null class/interface pointer could overwrite a previously evaluated entry in the vtbl->class name cache depending on what happens to be in an uninitialized local variable.
>
> Hopefully the last bug in that area, but you never know. Adding logging would help diagnosing bugs but would have to be more global.

Thanks.

I can't imagine though why adding a simple field would screw the code up.

e.g.,

class X {} // works.
class Y { int y; } // fails.

I would think that such things would have no effect on the pointer value?

Can you explain to me, for my own edification, why removing those non-essential lines would produce the effect?

On 28/07/2019 06:59, Bert wrote:
> On Wednesday, 24 July 2019 at 15:32:01 UTC, Rainer Schuetze wrote:
>>
>>
>> On 24/07/2019 08:33, Bert wrote:
>>> On Tuesday, 23 July 2019 at 20:57:17 UTC, Rainer Schuetze wrote
>>>> As it turned out, the problem was (again?) a false entry in the class
>>>> name cache that was added when an invalid pointer (including null) was
>>>> evaluated (and its dynamic was tried to be determined). Stupid bug, but
>>>> that happens.
>>>>
>>>> You can try the fixed version of MagoNatCC.dll from https://ci.appveyor.com/project/rainers/mago/build/artifacts
>>>
>>> That worked! I still find it very odd that it would exhibit it with
>>> irrelevant lines of code removed. Was that changing the cached
>>> version or something? Remember I basically have several Q's in my
>>> program all virtually identical and some would work and others wouldn't.
>>>
>>> Could you post some of the code that you fixed(or I guess push it to master) when you get a chance... I'm just curious to what it looks like. While such bugs are very annoying and better coding methods should be used, I imagine that this type of scenario for interface resolution is probably well localized and this would probably be the last bug fix for it? If not maybe some logging could be implemented. Interfaces should always be resolvable so if the code fails it is a bug in Visual D and having it logged in some way might help future cases.
>>
>> The bugfix commit is https://github.com/rainers/mago/commit/9801da089a4b4e9ffa8f1442f4b0aff5852d5fa9?w=1.
>>
>>
>> The problem was that evaluating the dynamic type of any uninitialized or null class/interface pointer could overwrite a previously evaluated entry in the vtbl->class name cache depending on what happens to be in an uninitialized local variable.
>>
>> Hopefully the last bug in that area, but you never know. Adding logging would help diagnosing bugs but would have to be more global.
> 
> Thanks.
> 
> I can't imagine though why adding a simple field would screw the code up.
> 
> e.g.,
> 
> class X {} // works.
> class Y { int y; } // fails.
> 

This wouldn't have failed, int needs a class/interface member, e.g.

class X
{
   Q q; // non-null
   I c; // null
   Q s; // non-null
}

> I would think that such things would have no effect on the pointer value?
> 
> Can you explain to me, for my own edification, why removing those non-essential lines would produce the effect?
> 

The bug basically was similar to

// global cache
string[void*] mapVtblToClassName;

void showX(X x)
{
   void* vtbl;
   string classNameq;
   bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
   if (rc)
   {
      if (vtbl in mapVtblToClassName)
      {
         classNameq = mapVtblToClassName[vtbl];
         goto Lnext;
      }
      classNameq = getClassNameFromVtbl(vtbl); // Q
   }
   mapVtblToClassName[vtbl] = classNameq;

Lnext:
   string classNamec;
   rc = ReadProcessMemory(x.c, vtbl); // fails, vtbl unchanged
   if (rc)
   {
      // same as above...
   }
   mapVtblToClassName[vtbl] = classNamec; // overwrites entry for Q
}

Next time a Q is evaluated, the determined class name will be empty.

On Sunday, 28 July 2019 at 11:45:21 UTC, Rainer Schuetze wrote:
>
>
> On 28/07/2019 06:59, Bert wrote:
>> On Wednesday, 24 July 2019 at 15:32:01 UTC, Rainer Schuetze wrote:
>>>
>>>
>>> On 24/07/2019 08:33, Bert wrote:
>>>> On Tuesday, 23 July 2019 at 20:57:17 UTC, Rainer Schuetze wrote
>>>>> As it turned out, the problem was (again?) a false entry in the class
>>>>> name cache that was added when an invalid pointer (including null) was
>>>>> evaluated (and its dynamic was tried to be determined). Stupid bug, but
>>>>> that happens.
>>>>>
>>>>> You can try the fixed version of MagoNatCC.dll from https://ci.appveyor.com/project/rainers/mago/build/artifacts
>>>>
>>>> That worked! I still find it very odd that it would exhibit it with
>>>> irrelevant lines of code removed. Was that changing the cached
>>>> version or something? Remember I basically have several Q's in my
>>>> program all virtually identical and some would work and others wouldn't.
>>>>
>>>> Could you post some of the code that you fixed(or I guess push it to master) when you get a chance... I'm just curious to what it looks like. While such bugs are very annoying and better coding methods should be used, I imagine that this type of scenario for interface resolution is probably well localized and this would probably be the last bug fix for it? If not maybe some logging could be implemented. Interfaces should always be resolvable so if the code fails it is a bug in Visual D and having it logged in some way might help future cases.
>>>
>>> The bugfix commit is https://github.com/rainers/mago/commit/9801da089a4b4e9ffa8f1442f4b0aff5852d5fa9?w=1.
>>>
>>>
>>> The problem was that evaluating the dynamic type of any uninitialized or null class/interface pointer could overwrite a previously evaluated entry in the vtbl->class name cache depending on what happens to be in an uninitialized local variable.
>>>
>>> Hopefully the last bug in that area, but you never know. Adding logging would help diagnosing bugs but would have to be more global.
>> 
>> Thanks.
>> 
>> I can't imagine though why adding a simple field would screw the code up.
>> 
>> e.g.,
>> 
>> class X {} // works.
>> class Y { int y; } // fails.
>> 
>
> This wouldn't have failed, int needs a class/interface member, e.g.
>
> class X
> {
>    Q q; // non-null
>    I c; // null
>    Q s; // non-null
> }
>
>> I would think that such things would have no effect on the pointer value?
>> 
>> Can you explain to me, for my own edification, why removing those non-essential lines would produce the effect?
>> 
>
> The bug basically was similar to
>
> // global cache
> string[void*] mapVtblToClassName;
>
> void showX(X x)
> {
>    void* vtbl;
>    string classNameq;
>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>    if (rc)
>    {
>       if (vtbl in mapVtblToClassName)
>       {
>          classNameq = mapVtblToClassName[vtbl];
>          goto Lnext;
>       }
>       classNameq = getClassNameFromVtbl(vtbl); // Q
>    }
>    mapVtblToClassName[vtbl] = classNameq;
>
> Lnext:
>    string classNamec;
>    rc = ReadProcessMemory(x.c, vtbl); // fails, vtbl unchanged
>    if (rc)
>    {
>       // same as above...
>    }
>    mapVtblToClassName[vtbl] = classNamec; // overwrites entry for Q
> }
>
> Next time a Q is evaluated, the determined class name will be empty.


Thanks. Ok, I see how the caching can be overwritten but I still don't see how changing a field in the class, which won't even modify the vtable, will effect the cache. I'd expect some sort of determinism in this, in that if I compile the some code and everything works and then simply add a field to a class and recompile, little should change.

E.g., if the cache is being overwritten in the first case then adding the field shouldn't change it from being overwritten in the second case.

Oh, nevermind,

ReadProcessMemory(x.q, vtbl);

you are iterating over the entries in x to get their vtable! So changing a field could cause a shift in the execution order and hence initiate the overwrite. (some of the code is missing above but I see how it could lead to these problems).

While I still don't fully understand why the code is doing what it does(e.g., q and c? I assume these are suppose to be members). Seems there doesn't need to be a goto and 2nd branch in theory but I guess it's incomplete code so I'll assume it all is suppose to work[Or you are just showing a second iteration].


Essentially just one part of the code is buggy because classNameq is empty and it can blindly overwrite the vtbl value *if* any future iterations occur where the if(rc) check fails(e.g., an added field).

>    void* vtbl;
>    string classNameq;
>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>    if (rc)
>    {
>       if (vtbl in mapVtblToClassName)
>       {
>          classNameq = mapVtblToClassName[vtbl];
>          goto Lnext;
>       }
>       classNameq = getClassNameFromVtbl(vtbl); // Q
>    }
>    mapVtblToClassName[vtbl] = classNameq;


which then something like

if (classNameq) mapVtblToClassName[vtbl] = classNameq;

would fix the problem?

On 01/08/2019 05:59, Bert wrote:
> On Sunday, 28 July 2019 at 11:45:21 UTC, Rainer Schuetze wrote:
>>
>>
>> On 28/07/2019 06:59, Bert wrote:
>>> I can't imagine though why adding a simple field would screw the code up.
>>>
>>> e.g.,
>>>
>>> class X {} // works.
>>> class Y { int y; } // fails.
>>>
>>
>> This wouldn't have failed, int needs a class/interface member, e.g.
>>
>> class X
>> {
>>    Q q; // non-null
>>    I c; // null
>>    Q s; // non-null
>> }
>>
>>> I would think that such things would have no effect on the pointer value?
>>>
>>> Can you explain to me, for my own edification, why removing those non-essential lines would produce the effect?
>>>
>>
>> The bug basically was similar to
>>
>> // global cache
>> string[void*] mapVtblToClassName;
>>
>> void showX(X x)
>> {
>>    void* vtbl;
>>    string classNameq;
>>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>>    if (rc)
>>    {
>>       if (vtbl in mapVtblToClassName)
>>       {
>>          classNameq = mapVtblToClassName[vtbl];
>>          goto Lnext;
>>       }
>>       classNameq = getClassNameFromVtbl(vtbl); // Q
>>    }
>>    mapVtblToClassName[vtbl] = classNameq;
>>
>> Lnext:
>>    string classNamec;
>>    rc = ReadProcessMemory(x.c, vtbl); // fails, vtbl unchanged
>>    if (rc)
>>    {
>>       // same as above...
>>    }
>>    mapVtblToClassName[vtbl] = classNamec; // overwrites entry for Q
>> }
>>
>> Next time a Q is evaluated, the determined class name will be empty.
> 
> 
> Thanks. Ok, I see how the caching can be overwritten but I still don't see how changing a field in the class, which won't even modify the vtable, will effect the cache. I'd expect some sort of determinism in this, in that if I compile the some code and everything works and then simply add a field to a class and recompile, little should change.
> 
> E.g., if the cache is being overwritten in the first case then adding the field shouldn't change it from being overwritten in the second case.
> 
> Oh, nevermind,
> 
> ReadProcessMemory(x.q, vtbl);
> 
> you are iterating over the entries in x to get their vtable! So changing a field could cause a shift in the execution order and hence initiate the overwrite. (some of the code is missing above but I see how it could lead to these problems).
> 
> While I still don't fully understand why the code is doing what it does(e.g., q and c? I assume these are suppose to be members). Seems there doesn't need to be a goto and 2nd branch in theory but I guess it's incomplete code so I'll assume it all is suppose to work[Or you are just showing a second iteration].

Yes, this was pseudo code showing the effect showing two calls to the same function. I used "goto" where a function would use "return".

> 
> 
> Essentially just one part of the code is buggy because classNameq is empty and it can blindly overwrite the vtbl value *if* any future iterations occur where the if(rc) check fails(e.g., an added field).
> 
>>    void* vtbl;
>>    string classNameq;
>>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>>    if (rc)
>>    {
>>       if (vtbl in mapVtblToClassName)
>>       {
>>          classNameq = mapVtblToClassName[vtbl];
>>          goto Lnext;
>>       }
>>       classNameq = getClassNameFromVtbl(vtbl); // Q
>>    }
>>    mapVtblToClassName[vtbl] = classNameq;
> 
> 
> which then something like
> 
> if (classNameq) mapVtblToClassName[vtbl] = classNameq;
> 
> would fix the problem?
> 
> 
> 

The empty classname is also cached for invalid vtbl pointers (e.g. when
reading uninitialized data). The fix is to move the insertion into the
map into the "if(rc)" branch.

On Friday, 2 August 2019 at 05:28:44 UTC, Rainer Schuetze wrote:
>
>
> On 01/08/2019 05:59, Bert wrote:
>> On Sunday, 28 July 2019 at 11:45:21 UTC, Rainer Schuetze wrote:
>>>
>>>
>>> On 28/07/2019 06:59, Bert wrote:
>>>> I can't imagine though why adding a simple field would screw the code up.
>>>>
>>>> e.g.,
>>>>
>>>> class X {} // works.
>>>> class Y { int y; } // fails.
>>>>
>>>
>>> This wouldn't have failed, int needs a class/interface member, e.g.
>>>
>>> class X
>>> {
>>>    Q q; // non-null
>>>    I c; // null
>>>    Q s; // non-null
>>> }
>>>
>>>> I would think that such things would have no effect on the pointer value?
>>>>
>>>> Can you explain to me, for my own edification, why removing those non-essential lines would produce the effect?
>>>>
>>>
>>> The bug basically was similar to
>>>
>>> // global cache
>>> string[void*] mapVtblToClassName;
>>>
>>> void showX(X x)
>>> {
>>>    void* vtbl;
>>>    string classNameq;
>>>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>>>    if (rc)
>>>    {
>>>       if (vtbl in mapVtblToClassName)
>>>       {
>>>          classNameq = mapVtblToClassName[vtbl];
>>>          goto Lnext;
>>>       }
>>>       classNameq = getClassNameFromVtbl(vtbl); // Q
>>>    }
>>>    mapVtblToClassName[vtbl] = classNameq;
>>>
>>> Lnext:
>>>    string classNamec;
>>>    rc = ReadProcessMemory(x.c, vtbl); // fails, vtbl unchanged
>>>    if (rc)
>>>    {
>>>       // same as above...
>>>    }
>>>    mapVtblToClassName[vtbl] = classNamec; // overwrites entry for Q
>>> }
>>>
>>> Next time a Q is evaluated, the determined class name will be empty.
>> 
>> 
>> Thanks. Ok, I see how the caching can be overwritten but I still don't see how changing a field in the class, which won't even modify the vtable, will effect the cache. I'd expect some sort of determinism in this, in that if I compile the some code and everything works and then simply add a field to a class and recompile, little should change.
>> 
>> E.g., if the cache is being overwritten in the first case then adding the field shouldn't change it from being overwritten in the second case.
>> 
>> Oh, nevermind,
>> 
>> ReadProcessMemory(x.q, vtbl);
>> 
>> you are iterating over the entries in x to get their vtable! So changing a field could cause a shift in the execution order and hence initiate the overwrite. (some of the code is missing above but I see how it could lead to these problems).
>> 
>> While I still don't fully understand why the code is doing what it does(e.g., q and c? I assume these are suppose to be members). Seems there doesn't need to be a goto and 2nd branch in theory but I guess it's incomplete code so I'll assume it all is suppose to work[Or you are just showing a second iteration].
>
> Yes, this was pseudo code showing the effect showing two calls to the same function. I used "goto" where a function would use "return".
>
>> 
>> 
>> Essentially just one part of the code is buggy because classNameq is empty and it can blindly overwrite the vtbl value *if* any future iterations occur where the if(rc) check fails(e.g., an added field).
>> 
>>>    void* vtbl;
>>>    string classNameq;
>>>    bool rc = ReadProcessMemory(x.q, vtbl); // reads vtbl-ptr
>>>    if (rc)
>>>    {
>>>       if (vtbl in mapVtblToClassName)
>>>       {
>>>          classNameq = mapVtblToClassName[vtbl];
>>>          goto Lnext;
>>>       }
>>>       classNameq = getClassNameFromVtbl(vtbl); // Q
>>>    }
>>>    mapVtblToClassName[vtbl] = classNameq;
>> 
>> 
>> which then something like
>> 
>> if (classNameq) mapVtblToClassName[vtbl] = classNameq;
>> 
>> would fix the problem?
>> 
>> 
>> 
>
> The empty classname is also cached for invalid vtbl pointers (e.g. when
> reading uninitialized data). The fix is to move the insertion into the
> map into the "if(rc)" branch.


Ok, thanks. At least it makes it clear how these errors can happen.

On Friday, 31 May 2019 at 16:30:02 UTC, Rainer Schuetze wrote:
> Hi,
>
> for the adventurous: I have just released a preliminary version of Visual D with some new major changes:
>
> - there is now another installer available that includes DMD and LDC, so no additional setup steps necessary to get you going
>
> - now checks for updates for Visual D, DMD and LDC and assists with downloading and installing
>
> - experimental: highlight references to the same symbol as the one at the caret (to be enabled in the editor options)
>
> - mago debugger extension: better dynamic type handling of classes and interfaces, show exception message
>
> Grab it here: https://github.com/dlang/visuald/releases/tag/v0.50.0-beta1
>
> Cheers,
> Rainer

thank you