Refactoring: Make dmd.root.file.File stateless

[kinke]

[I had to look into this, as we get an ICE with -lowmem on Linux/i386, a segfault originating from a File dtor.]

[dlang-bot]

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9728"

[thewilsonator]

@kinke is this good to go?

[kinke]

Nope, not ready; e.g., the autotester suffers from the 2.079 DMD host compiler with its size_t C++-mangling regression on OSX64 (fixed in 2.082...); i.e., d_size_t doesn't account for DMD 2.079-2.081 host compilers on OSX. That's most likely the reason for the previous duplicate code (method also defined inline in C++ header) which I don't want to keep just to hack around the C++ frontend tests.

[kinke]

I'm surprised about the review action taken here - don't get me wrong, I appreciate it, just definitely not used to that level of detail. ;)

This started out as a small refactoring to aid me in analysing the ownership of these buffers and how on earth the dtor using mem.xfree wasn't an issue until now (before -lowmem and enabled GC), although read() uses C malloc. I think I found the bug - a File representing an object file assumed ownership of a backend Outbuffer (don't confuse with frontend OutBuffer; the latter using mem, the former C) just before writing it to disk; the instance lived on the heap and so wasn't finalized/destructed without enabled GC. Edit: That was a bug, but cannot be the LDC bug...

I don't have the energy for a proper refactoring - I think there should be no File (filename + buffer) at all. The buffer is only used transiently, either to read an existing file and then taking ownership away, or to set a buffer right before writing it out. The cognitive overhead of the buffer ownership would IMO be best mitigated by simply having something like Array!ubyte readFile(string filename) and void writeFile(string filename, void[] data).

[WalterBright]

I think this conversation shows a bit of the downside of combining a bug fix with refactoring. They really should be separate ones. It's a little hard to see the bug fix in this refactoring.

As for refactoring, I really wish to raise the bar on what is a good refactoring. I've written a bit of a manifesto on it #9511. I'm not singling you out, consider #9729 where I put @benjones through the wringer. There's a lot of technical debt in dmd, and I don't want to have to refactor the refactors, so they have to be the best we can do. It had better be good if it says "cleanup" in the title :-)

[WebDrake]

Just to give input here -- note that this ICE is blocking an i386 snap package release for LDC 1.15.0, and this could mean that something is broken also for i386 DMD 2.085.1. Is there any chance of the fix making it into a DMD 2.085.2 (which would allow a corresponding LDC 1.15.1)?

[kinke]

DMD got -lowmem with 2.086, so 2.085 isn't affected; and we don't need a fix to land in DMD first to cherry-pick it to LDC.

[WalterBright]

The trouble with this PR is it combined refactoring with the bug fix. They need to be SEPARATE PRs. The refactoring is what is delaying approving this. The bug fix itself is not controversial.

Please, everyone, do not mix refactoring with bug fixes.

[WebDrake]

The trouble with this PR is it combined refactoring with the bug fix. They need to be SEPARATE PRs.

Completely agree that refactoring and fixes should not be combined. Sorry if my feedback came over as pressure or impatience -- it was related purely to the target branch for the fix, to ensure we got it everywhere that was needed. But @kinke's response allays those concerns.

[kinke]

I split off the fixes to #9771, targeting the stable branch. - I'll rebase this after that is in master.

[thewilsonator]

You can make a stable -> master PR with this script

[kinke]

Reduced to just a squashed refactoring. Semaphore failure seems unrelated.

comments addressed.

[ibuclaw]

REGRESSION, you can no longer have a module named __main.d in any project.

This also breaks gdc, where __main.d implementation is not embedded inside the compiler.

[kinke]

You're saying gdc itself uses a file named __main.d, without any path? My goal here was to move the special files to one place, instead of handling __stdin.d here and setting up __main.d in mars.d (the only place where the file buffer was set manually from outside and then File.read() being a no-op).

[ibuclaw]

$ echo "import std.stdio; int main() { writeln(\"Hello\"); return 42; }" > __main.d
$ ./generated/linux/release/64/dmd __main.d -ofmain
$ ./main 
$ echo $?
0

How exactly is this desirable behaviour?

IMO, root/file.d should just be a generic wrapper around a file, it should not have any compiler-specific logic embedded within.

[kinke]

This applies to __stdin.d as well then.

[kinke]

=> #9795