www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Use-after-scope bug in Phobos unittest, how to fix?

reply Johan <j j.nl> writes:
Hi all,
   I found a bug in Phobos typecons unittest:
https://github.com/ldc-developers/phobos/blob/6c83b490f7d6c66bf430e5249dae608848d3ac2c/std/typecons.d#LL7088C1-L7108C46
```d
pure  system unittest
{
     foreach (MyRefCounted; AliasSeq!(SafeRefCounted, RefCounted))
     {
         MyRefCounted!int* p;
         {
             auto rc1 = MyRefCounted!int(5);
             p = &rc1; // assigns reference to variable in inner 
scope...
             assert(rc1 == 5);
             assert(rc1._refCounted._store._count == 1);
             auto rc2 = rc1;
             assert(rc1._refCounted._store._count == 2);
             // Reference semantics
             rc2 = 42;
             assert(rc1 == 42);
             rc2 = rc2;
             assert(rc2._refCounted._store._count == 2);
             rc1 = rc2;
             assert(rc1._refCounted._store._count == 2);
         }
         assert(p._refCounted._store == null);  // use after scope!
```

The bug is uncovered when optimization and variable lifetime are 
considered with LDC.
I see that the test is trying to prove that the MyRefCounted 
dtors are run correctly when the scope ends. However, the way 
that this is tested is technically UB as far as I understand the 
D lang spec (although I cannot find explicit mention of variable 
lifetimes, please help...).
I can simply disable optimization for this unittest function 
(ldc.attributes.optStrategy), but would rather not.

Ideas?

thanks,
   Johan
May 15 2023
next sibling parent reply "Richard (Rikki) Andrew Cattermole" <richard cattermole.co.nz> writes:
You should probably disable such optimizations for  system code 
entirely. Not sure about  trusted, don't think it should apply there either.

Having optimizations that allow for UB or crashing at runtime is not a 
good thing.
May 15 2023
parent reply Johan <j j.nl> writes:
On Monday, 15 May 2023 at 18:59:12 UTC, Richard (Rikki) Andrew 
Cattermole wrote:

 You should probably disable such optimizations for  system code 
 entirely. Not sure about  trusted, don't think it should apply 
 there either.

 Having optimizations that allow for UB or crashing at runtime 
 is not a good thing.


I think there is a misunderstanding here. The use-after-scope UB 
of the unittest is UB regardless of optimization. Optimization 
(when correct) doesn't change the validity of the program.
If you don't want optimizations uncovering latent UB bugs, then 
you should not use an optimizing compiler. ;)

-Johan
May 15 2023
parent reply "Richard (Rikki) Andrew Cattermole" <richard cattermole.co.nz> writes:
Just to confirm something, you're saying we can't use alloca when 
optimizing with ldc? Because this is what that code essentially is doing 
(which could be perfectly fine for  system to do).
May 15 2023
parent Johan <j j.nl> writes:
On Monday, 15 May 2023 at 19:27:00 UTC, Richard (Rikki) Andrew 
Cattermole wrote:

 Just to confirm something, you're saying we can't use alloca 
 when optimizing with ldc?


No.


 Because this is what that code essentially is doing (which 
 could be perfectly fine for  system to do).


The code is not calling `alloca`. It is defining a local variable 
in some inner scope, then exits that scope, then accesses the 
out-of-scope variable through a pointer.
It is irrelevant how local variable memory is allocated (could be 
"stack" or "heap" or some other mechanism, which are 
implementation details that may differ and are irrelevant to the 
UB).

-Johan
May 15 2023
prev sibling next sibling parent user456 <user456 123.de> writes:
On Monday, 15 May 2023 at 18:55:09 UTC, Johan wrote:

 Hi all,
   I found a bug in Phobos typecons unittest:
 https://github.com/ldc-developers/phobos/blob/6c83b490f7d6c66bf430e5249dae608848d3ac2c/std/typecons.d#LL7088C1-L7108C46
 ```d
 pure  system unittest
 {
     foreach (MyRefCounted; AliasSeq!(SafeRefCounted, 
 RefCounted))
     {
         MyRefCounted!int* p;
         {
             auto rc1 = MyRefCounted!int(5);
             p = &rc1; // assigns reference to variable in inner 
 scope...
             assert(rc1 == 5);
             assert(rc1._refCounted._store._count == 1);
             auto rc2 = rc1;
             assert(rc1._refCounted._store._count == 2);
             // Reference semantics
             rc2 = 42;
             assert(rc1 == 42);
             rc2 = rc2;
             assert(rc2._refCounted._store._count == 2);
             rc1 = rc2;
             assert(rc1._refCounted._store._count == 2);
         }
         assert(p._refCounted._store == null);  // use after 
 scope!
 ```

 [...]

 Ideas?

 thanks,
   Johan


I'd simply suggest to remove the assertion or to put it in the 
inner scope given that the problem is presumably that all allocas 
for the unittest are done on function entry
May 15 2023
prev sibling parent reply Dennis <dkorpel gmail.com> writes:
On Monday, 15 May 2023 at 18:55:09 UTC, Johan wrote:

 I see that the test is trying to prove that the MyRefCounted 
 dtors are run correctly when the scope ends.


That the destructor is run at the end of the scope is the job of 
the compiler, not something that this unittest needs to test. To 
test that the destructor nullifies the `store`, `__dtor` can be 
called manually.
May 15 2023
parent Johan <j j.nl> writes:
On Monday, 15 May 2023 at 20:22:15 UTC, Dennis wrote:

 On Monday, 15 May 2023 at 18:55:09 UTC, Johan wrote:

 I see that the test is trying to prove that the MyRefCounted 
 dtors are run correctly when the scope ends.


 That the destructor is run at the end of the scope is the job 
 of the compiler, not something that this unittest needs to 
 test. To test that the destructor nullifies the `store`, 
 `__dtor` can be called manually.


https://github.com/dlang/phobos/pull/8751
May 16 2023