Clean up stale split debuginfo in incremental builds

[orlp]

Fixes #161824.

LLM disclosure: I heavily used LLMs to navigate the codebase and suggest prototype solutions, but every line of code in the end result is written by me.

For the reviewer: there is one snag I don't fully grasp. This PR changes the WorkProduct struct which I believe is serialized / deserialized. Is some sort of migration necessary so old WorkProducts deserialize correctly?

[rustbot]

r? @mu001999

rustbot has assigned @mu001999.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, incremental
• compiler, incremental expanded to 76 candidates
• Random selection from 22 candidates

[orlp]

r? @bjorn3

(Since you responded on the issue.)

[bjorn3]

Is some sort of migration necessary so old WorkProducts deserialize correctly?

Rustc never reads incr comp caches from different rustc versions.

[bjorn3]

I feel like it would be better if we instead put all debuginfo in a new crate specific directory inside the output directory and then just clear out this directory right before codegen (or after codegen remove all files not listed as dwarf_object in the CompiledModules). That way stale debuginfo can also be removed for non-incremental builds when the amount/names of codegen units changes and we don't need to persistently track this info. If anything I would like to remove WorkProduct entirely to ensure we can't accidentally forget to track a work product in the incr comp session. Instead just build a new incr comp session dir from scratch every time (#159287) and copy the latest version of a work product into it at a location derived from the cgu name.

[orlp]

@bjorn3 That sounds like it could work as well. Perhaps that PR needs a new reviewer or a gentle ping? I don't think it has seen any review work since its conception in July.

[bjorn3]

That PR is blocked on me resolving some review comments that @nnethercote passed me in private after asking if was fine with an LLM review. I resolved most already, but one is a bit more tricky. (fcntl(F_SETLK) was a mistake. Unix should have used flock() from the start.) I just need to find some time to finish it.

That PR shouldn't be necessary for moving unpacked debuginfo into a separate directory.

[orlp]

That PR shouldn't be necessary for moving unpacked debuginfo into a separate directory.

I don't understand this. As far as I understand the debuginfo files need to be right next to the object files otherwise the debugger can't find them. So either you move everything or nothing.

You could probably move them and update the paths with some kind of arguments to the linker, but that's also more involved.

[bjorn3]

As far as I understand the debuginfo files need to be right next to the object files otherwise the debugger can't find them.

You could also move the object files into this dir if necessary. They are removed after linking anyway (unless -Csave-temps is used, but that is only for debugging anyway). In any case my understanding is that the object files embed a path to the separate debuginfo files which can be either relative or absolute. So I think just adjusting the path where LLVM should write the dwo file is enough to make it include the correct relative path in the object file. Adjusting temp_path_dwo_for_cgu (+ creating the debuginfo directory first) may be enough.

You could probably move them and update the paths with some kind of arguments to the linker, but that's also more involved.

The linker knows nothing about debuginfo. It just copies .debug_* sections into the output executable by default.

[epage]

Instead of clearing content, shouldn't we drop the invocation time? At the point in time in question (build complete), they aren't temp files and it seems like it would be good for build systems to not include timestamps in the output, even for debug info.

[bjorn3]

The invocation temp is a workaround for an incr comp bug (reusing the same name causes the incr comp cache to get corrupted due to writes through hardlinks). #159287 may allow getting rid of the invocation temp if I understand correctly why it is necessary. And even without it, changing the cgu paritioning would still leave outdated cgu debuginfo on disk currently.

[epage]

The invocation temp is a workaround for an incr comp bug (reusing the same name causes the incr comp cache to get corrupted due to writes through hardlinks). #159287 may allow getting rid of the invocation temp if I understand correctly why it is necessary.

Why is incr comp cache tied to naming of split debug info?

And even without it, changing the cgu paritioning would still leave outdated cgu debuginfo on disk currently.

Like changing the number of codegen units? Is that rustc or the build tools job? For cargo, I assume codegen units is in the unit hash so changing it gets a new cache entry, instead of modifying an existing one. That becomes more relevant with the newly stabilized build dir layout becaus each unit hash's rustc output is put in a dedicated directory.

[bjorn3]

Why is incr comp cache tied to naming of split debug info?

The unpacked split debuginfo just leaves some temporary files that would otherwise be cleaned up in the outdir. It doesn't do an extra copy that could rename them. The incr comp cache contains hardlinks with the same name as the temporary files that are first written outside the incr comp cache. As I understand the reason the invocation temp is necessary is that when LLVM writes artifacts, it doesn't remove the old file and writes a new file. Rather it truncates the file and writes to the existing file. For unpacked split debuginfo this would truncate the file hardlinked into the previous incr comp cache session too, thereby corrupting it. By using a unique name every time, no existing file gets overwritten.

Like changing the number of codegen units? Is that rustc or the build tools job? For cargo, I assume codegen units is in the unit hash so changing it gets a new cache entry, instead of modifying an existing one. That becomes more relevant with the newly stabilized build dir layout becaus each unit hash's rustc output is put in a dedicated directory.

-Ccodegen-units places an upper bound on the amount of cgus. Especially with incr comp where the default upper bound is 256, most of the time rustc will not hit this upper bound as there are not enough modules in the source code to hit this upper bound during initial partitioning before merging based on the upper bound. So if a crate had say 12 cgus and one module got removed on the next rebuild, the foo.abcd.12.rcgu.dwo file would remain. In addition we only use numbered cgu names when incr comp is disabled. When it is enabled, the names of the source modules are hashed together to form the cgu name. So any module rename or change in merging between modules would leave stale files.

[epage]

Thanks for the clarification.

So it sounds like this is a workaround in incr comp that leaks out due to some implementation details?

And as for the name stability, sounds like names would otherwise be deterministic, even if not exactly the same every time. It sounds like the growth in that case is much much smaller than the growth we are currently seeing and I'm not saying we should only do one or the other. What I'm wondering about for clean up is if Cargo should do it anyways before a build. If that is the case, should rustc also be doing clean up or leave it to build systems? Resolving the non-deterministic names solves a problem and, I'm assuming, dramatically reduces the impact of leakage so we can answer the clean up question.