only return used dependencies in crate_dependencies_in_postorder

[lqd]

PR #124976 made use of used_crates and not all crates including speculatively loaded. This apparently broke some cases of trying to link (e.g. with upstream rust crates) which seems to expect all crates instead by doing index-based accesses in now-different arrays, and caused ICEs #125474 and #125484.

We have 3 repros:

• using a proc-macro with a dependency: MCVE from ICE: index out of bounds in rustc_codegen_ssa #125474 in https://github.com/parasyte/crashy
• using a collision between a module and a dependency: MCVE from ICE when using Instant type with the time crate. #125484 (comment). I don't know what's going there, but this PR fixes it. The collision may be incidental.
• using a re-export from time: repro from ICE when using Instant type with the time crate. #125484 (comment). Has proc-macros and IIUC name collisions, so it could be a mix between the above two.

These ICEs are fixed by this PR BUT I can't get compiletest to emit the command line I'm seeing in the repros, with neither of the 2 MCVEs. Naively using aux-builds and so on doesn't reproduce the ICE. (It's also incredibly hard to make it show the command used to build the aux-build files). So I had to use a run-make test instead, in order to have at least one test in this PR.

r? @oli-obk as #124976's reviewer or @petrochenkov as #124976's author

fixes #125474
fixes #125484
fixes #125646
fixes #125707

[petrochenkov]

I think the postorder_cnums query is to blame here.
It should work as postorder_cnums_used in this context, but it actually works as postorder_cnums_including_speculative.

[lqd]

ok I'll look into that

@rustbot author

[lqd]

@petrochenkov Right, good spot.

These queries and code could use some comments and renames to clarify these behaviors.

@rustbot ready

Still would like to have tests :/

[petrochenkov]

All direct and indirect users of crate_dependencies_in_postorder do happen to need the "used" semantics, but that's certainly not clear from the naming.

[rustbot]

Some changes occurred in run-make tests.

cc @jieyouxu

[lqd]

For the life of me I can't wrestle compiletest annotations to make a test and may be missing something obvious that oli/vadim would see. Plan B: I pushed a run-make test to actually reproduce the ICE.

[jieyouxu]

I couldn't get a UI test to work either so I think a run-make test is fine :3 Might be related to -L flags, but I didn't dig very deep.

[bjorn3]

The simple-raytracer benchmark of cg_clif is another reproducer for this.

[petrochenkov]

r? @petrochenkov
@bors r+

Instead of reverting #124976 (#124976 (comment)).

[bors]

📌 Commit f225db1 has been approved by petrochenkov

It is now in the queue for this repository.

[lqd]

@petrochenkov I don't think this PR fixes all open issues

[petrochenkov]

Ah, okay, then let's revert #124976 too (at least for beta).

[lqd]

@petrochenkov I won't have a lot of time to analyze and fix the other issues in the next few days.

So I feel it'd be safer to:

• do the revert now, on nightly before the beta cut, rather than land this PR
• add some assertions that the different arrays have the same lengths, analyze and fix the couple different issues (but 6-7 including duplicates overall)
• reland 124976 (maybe w/ a crater run)

what do you think?

(but I can tell you that reverting the same commits as I did in ad545ac makes the ICEs from #126021 disappear. I can't reproduce the exact same context, and am not sure about correctness here, e.g. it seems likely the indices could still be in-bounds but wrong)

[lqd]

Until I hear back from vadim:

@bors r-

[bors]

☔ The latest upstream changes (presumably #126097) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@lqd any updates on replying to #125493 (comment) ? thanks

[Dylan-DPC]

oops wrong ping, meant to ping @petrochenkov :D

[lqd]

We did the revert, and we’ll need part of this PR when we want to reland the original changes