www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - [SAOC 2024] - Fix Segmentation fault when compiling druntime Weekly

reply Dennis <dennis.onyeka.4 gmail.com> writes:



After moving the newScope function (which is a semantic import) 
from attrib.d to dsymbolsem.d, and turning it into a visitor. I 
encountered an error with the following description:
```make -C druntime
make[2]: Entering directory '/home/dencomac/DL/dmd/druntime'
../generated/linux/release/64/dmd -c -conf= -Isrc -Iimport -w -de 
-preview=dip1000 -preview=fieldwise -m64 -fPIC 
-preview=dtorfields -O -release -inline -version=Shared -fPIC 
-version=Shared -fPIC -I. -P=-I. src/core/stdc/errno.c 
-of../generated/linux/release/64/errno_c.o
make[2]: *** [Makefile:394: 
../generated/linux/release/64/errno_c.o] Segmentation fault
make[2]: Leaving directory '/home/dencomac/DL/dmd/druntime'
make[1]: *** [Makefile:89: druntime] Error 2
make[1]: Leaving directory '/home/dencomac/DL/dmd'
make: *** [posix.mak:8: all] Error 2
```
It became a challenge to me and to the reviewers.  It was a 
runtime error. Although the dmd compiler was built successfully, 
when druntime(core runtime library that provides essential 
low-level support for the D) was tested, it failed.
Druntime, in combination with the D standard library Phobos, 
provides a complete ecosystem for building D applications.

It was later found out that the DeprecatedDeclaration function 
was not properly type casted.

DeprecatedDeclaration inherits from StorageClassDeclaration. This 
allows the function to access properties or methods that are 
specific to StorageClassDeclaration.
**Error code:**
```
override void visit(DeprecatedDeclaration dpd) {
auto scx = (cast(StorageClassDeclaration)dpd).newScope(sc); // 
The enclosing scope is deprecated as well
  if (scx == sc)
  scx = sc.push();
scx.depdecl = dpd;
sc = scx;
}
```


The code above casts dpd (of type DeprecatedDeclaration) to 
StorageClassDeclaration and then tries to call newScope on it.  
The cast failed at runtime because dpd does not have a relation 
to StorageClassDeclaration. That resulted in a segfault.

**Modified code:**
```
override void visit(DeprecatedDeclaration dpd)
{
	auto oldsc = sc;
	visit((cast(StorageClassDeclaration)dpd));  // Delegates to the 
parent visit method
	auto scx = sc;
	sc = oldsc; // Restores the original scope

	// The enclosing scope is deprecated as well
	if (scx == sc)
     	scx = sc.push(); // Pushes a new scope if no changes have 
been made
	scx.depdecl = dpd;
	sc = scx;
}
```
sc is saved into oldsc before any changes are made. Thereafter, 
`visit((cast(StorageClassDeclaration)dpd))` calls the parent 
visit() method for `StorageClassDeclaration,` allowing for any 
scope adjustments associated with this type.
The updated scope is now stored in sc.
The main aim was to restore the scope where appropriate.


https://github.com/dlang/dmd/pull/16880)](
https://github.com/dlang/dmd/pull/16880)
[](https://github.com/dlang/dmd/pull/16880)


The error took most of my time and made me research a little on 
what segmentation error and type casting in D lang is, with the 
help of a youtube video from Mike shah[ **[Dlang Episode 47] D 
Language -  Casting with 
‘cast’**](https://www.youtube.com/watch?v=ZTK9FhAS-mM)


In conclusion, segmentation fault occurs when a program tries to 
access memory that it is not authorized to access, or that does 
not exist. Some scenarios that can cause it is as follows: 
**Modifying a String Literal, Accessing an Address that is Freed, 
invalid cast OR Dereferencing Uninitialized.**


Completely remove all other functions that depends on dsymbolsem 
in attrib.d and move to Cond.d refactoring although the previous 
PR has been merged but a lot a work needs to be done.
Oct 06
next sibling parent reply "Richard (Rikki) Andrew Cattermole" <richard cattermole.co.nz> writes:
One way to handle this, would be to turn on ASAN (address sanitizer) 
temporarily.

https://johanengelen.github.io/ldc/2017/12/25/LDC-and-AddressSanitizer.html

If you have debug info, it'll output a lot of helpful information when 
an error occurs.
Oct 06
parent Dennis <dennis.onyeka.4 gmail.com> writes:
On Sunday, 6 October 2024 at 23:04:21 UTC, Richard (Rikki) Andrew 
Cattermole wrote:
 One way to handle this, would be to turn on ASAN (address 
 sanitizer) temporarily.

 https://johanengelen.github.io/ldc/2017/12/25/LDC-and-AddressSanitizer.html

 If you have debug info, it'll output a lot of helpful 
 information when an error occurs.
Ohh thanks. I'll look into it.
Oct 06
prev sibling next sibling parent reply Johan <j j.nl> writes:
On Sunday, 6 October 2024 at 22:25:30 UTC, Dennis wrote:



 After moving the newScope function (which is a semantic import) 
 from attrib.d to dsymbolsem.d, and turning it into a visitor. I 
 encountered an error with the following description:
 ```make -C druntime
 make[2]: Entering directory '/home/dencomac/DL/dmd/druntime'
 ../generated/linux/release/64/dmd -c -conf= -Isrc -Iimport -w 
 -de -preview=dip1000 -preview=fieldwise -m64 -fPIC 
 -preview=dtorfields -O -release -inline -version=Shared -fPIC 
 -version=Shared -fPIC -I. -P=-I. src/core/stdc/errno.c 
 -of../generated/linux/release/64/errno_c.o
 make[2]: *** [Makefile:394: 
 ../generated/linux/release/64/errno_c.o] Segmentation fault
 make[2]: Leaving directory '/home/dencomac/DL/dmd/druntime'
 make[1]: *** [Makefile:89: druntime] Error 2
 make[1]: Leaving directory '/home/dencomac/DL/dmd'
 make: *** [posix.mak:8: all] Error 2
 ```
 It became a challenge to me and to the reviewers.  It was a 
 runtime error. Although the dmd compiler was built 
 successfully, when druntime(core runtime library that provides 
 essential low-level support for the D) was tested, it failed.
Hi Dennis, all, I'm sorry to say, but I am not feeling very happy about this project. It is a very invasive change to the compiler, that I feel should not be undertaken by someone with relatively little experience in D, let alone D compiler frontend development. What are the safety checks to prevent _any_ actual change in compiler output? The test suite of the compiler (and druntime, and Phobos) is *very* scant and lacks rigorous testing in many places. Which means that you cannot rely on the testsuite to verify that indeed no change happened to the compiler output. There is the project builder (buildkite?) which expands the testsuite, that's nice, but but still. Are there any extra checks done to see whether binary output of the compiler has changed upon any PR submitted? (for a number of large code bases?) -Johan
Oct 07
next sibling parent reply Dom Disc <dominikus scherkl.de> writes:
On Monday, 7 October 2024 at 18:37:31 UTC, Johan wrote:
 It is a very invasive change to the compiler, that I feel 
 should not be undertaken by someone with relatively little 
 experience in D, let alone D compiler frontend development.
 What are the safety checks to prevent _any_ actual change in 
 compiler output?
?!? I would expect this project to indeed change the compiler output. To make the error messages more concise and easier to improve in the future. Why else do we want this? But I agree that it should only be integrated together with editions.
Oct 08
parent RazvanN <razvan.nitu1305 gmail.com> writes:
On Tuesday, 8 October 2024 at 08:50:24 UTC, Dom Disc wrote:
 On Monday, 7 October 2024 at 18:37:31 UTC, Johan wrote:
 It is a very invasive change to the compiler, that I feel 
 should not be undertaken by someone with relatively little 
 experience in D, let alone D compiler frontend development.
 What are the safety checks to prevent _any_ actual change in 
 compiler output?
?!? I would expect this project to indeed change the compiler output. To make the error messages more concise and easier to improve in the future. Why else do we want this? But I agree that it should only be integrated together with editions.
I think you are confusing this project with another one. This project is about extracting something routines out of AST nodes.
Oct 08
prev sibling parent reply RazvanN <razvan.nitu1305 gmail.com> writes:
On Monday, 7 October 2024 at 18:37:31 UTC, Johan wrote:
 On Sunday, 6 October 2024 at 22:25:30 UTC, Dennis wrote:



 After moving the newScope function (which is a semantic 
 import) from attrib.d to dsymbolsem.d, and turning it into a 
 visitor. I encountered an error with the following description:
 ```make -C druntime
 make[2]: Entering directory '/home/dencomac/DL/dmd/druntime'
 ../generated/linux/release/64/dmd -c -conf= -Isrc -Iimport -w 
 -de -preview=dip1000 -preview=fieldwise -m64 -fPIC 
 -preview=dtorfields -O -release -inline -version=Shared -fPIC 
 -version=Shared -fPIC -I. -P=-I. src/core/stdc/errno.c 
 -of../generated/linux/release/64/errno_c.o
 make[2]: *** [Makefile:394: 
 ../generated/linux/release/64/errno_c.o] Segmentation fault
 make[2]: Leaving directory '/home/dencomac/DL/dmd/druntime'
 make[1]: *** [Makefile:89: druntime] Error 2
 make[1]: Leaving directory '/home/dencomac/DL/dmd'
 make: *** [posix.mak:8: all] Error 2
 ```
 It became a challenge to me and to the reviewers.  It was a 
 runtime error. Although the dmd compiler was built 
 successfully, when druntime(core runtime library that provides 
 essential low-level support for the D) was tested, it failed.
Hi Dennis, all, I'm sorry to say, but I am not feeling very happy about this project. It is a very invasive change to the compiler, that I feel should not be undertaken by someone with relatively little experience in D, let alone D compiler frontend development. What are the safety checks to prevent _any_ actual change in compiler output? The test suite of the compiler (and druntime, and Phobos) is *very* scant and lacks rigorous testing in many places. Which means that you cannot rely on the testsuite to verify that indeed no change happened to the compiler output. There is the project builder (buildkite?) which expands the testsuite, that's nice, but but still. Are there any extra checks done to see whether binary output of the compiler has changed upon any PR submitted? (for a number of large code bases?) -Johan
Hi Johan, If you are arguing that the test suite is not good enough to catch breaking changes then that is an argument that can be brought up for literally any change that is brought to the compiler. In that regard I fail to see how this project is any different from any other modification. With respect to the type of change: I have done literally tens or even hundreds of such refactorings in the past 6-7 years. From my experience: (1) 0 regressions or bugs were reported linked to that sort of changes (apart from some C++ header mismatches - but I don't count those since those are not actually related to the compiler behavior); (2) if you break something while refactoring code - the test suite will catch it (if the compiler does not beat you up to it). Now, with regards to Dennis's experience: I am his SAoC mentor and I'm carefully reviewing his PRs before merging so please rest assured that care is being taken with this work. The only reason that I've submitted this project to SAoC is because it is something trivial enough that most experienced compiler devs avoid doing and because it is a good way for someone to get accustomed to the compiler codebase. Regards, RazvanN
Oct 08
next sibling parent Johan <j j.nl> writes:
On Tuesday, 8 October 2024 at 13:04:40 UTC, RazvanN wrote:
 If you are arguing that the test suite is not good enough to 
 catch breaking changes then that is an argument that can be 
 brought up for literally any change that is brought to the 
 compiler. In that regard I fail to see how this project is any 
 different from any other modification.
My concern is that this specific project touches the heart of the compiler. That is different from a project that would only touch a small part of the code, or would only be invoked on special occasion (e.g. an AST text writer). -Johan
Oct 08
prev sibling parent Steven Schveighoffer <schveiguy gmail.com> writes:
On Tuesday, 8 October 2024 at 13:04:40 UTC, RazvanN wrote:
 On Monday, 7 October 2024 at 18:37:31 UTC, Johan wrote:
 It is a very invasive change to the compiler, that I feel 
 should not be undertaken by someone with relatively little 
 experience in D, let alone D compiler frontend development.
 What are the safety checks to prevent _any_ actual change in 
 compiler output?
Hi Johan, If you are arguing that the test suite is not good enough to catch breaking changes then that is an argument that can be brought up for literally any change that is brought to the compiler. In that regard I fail to see how this project is any different from any other modification. With respect to the type of change: I have done literally tens or even hundreds of such refactorings in the past 6-7 years. From my experience: (1) 0 regressions or bugs were reported linked to that sort of changes (apart from some C++ header mismatches - but I don't count those since those are not actually related to the compiler behavior); (2) if you break something while refactoring code - the test suite will catch it (if the compiler does not beat you up to it).
I think Johan's point is that you are refactoring the compiler. If you are refactoring the compiler, the refactored compiler should produce the same output as the original compiler. A refactoring should not modify behavior (even to fix bugs), it should just make the code easier to use/write. Do we have any way to test this? Possibly we should compare disassembly? This would not be a CI task though - as it's impossible to pin down an "expected" disassembly for various projects. -Steve
Oct 11
prev sibling parent Mike Shah <mshah.475 gmail.com> writes:
On Sunday, 6 October 2024 at 22:25:30 UTC, Dennis wrote:
 [...]
Nice work!
Oct 07