Thread overview
[SAOC 2024] - Fix Segmentation fault when compiling druntime Weekly update #3
Oct 06
Dennis
Oct 06
Dennis
Oct 07
Johan
Oct 08
Dom Disc
Oct 08
RazvanN
Oct 08
RazvanN
Oct 08
Johan
Oct 08
Mike Shah
October 06

Fix Segmentation when Compiling druntime

Task Accomplished

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[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.

What do I mean?

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.

Commit link

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’

Summary

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.

Next on the task

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.

October 07
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.
October 06
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.
October 07

On Sunday, 6 October 2024 at 22:25:30 UTC, Dennis wrote:

>

Fix Segmentation when Compiling druntime

Task Accomplished

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[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

October 08

On Sunday, 6 October 2024 at 22:25:30 UTC, Dennis wrote:

>

[...]

Nice work!

October 08

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.

October 08

On Monday, 7 October 2024 at 18:37:31 UTC, Johan wrote:

>

On Sunday, 6 October 2024 at 22:25:30 UTC, Dennis wrote:

>

Fix Segmentation when Compiling druntime

Task Accomplished

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[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

October 08

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.

October 08

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

October 12

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