Repository navigation
[goal] modifying private method in another crate's impl should not require users of public methods to recompile #37333
Description
This is an extension of #37121 but to support modification of a private method in another crate. This is currently complicated by the fact that when we write metadata, we make one metadata node per item. So any changes to the item at all are confounded together. IOW, we can't tell the difference between adding an inherent impl or method to a struct and changing the types of its fields. This results in really poor re-use.
Activity
nikomatsakis commentedon Oct 21, 2016
cc @michaelwoerister
nikomatsakis commentedon Oct 21, 2016
As far as priorities, I would put this goal somewhat secondary -- it's very important, but we should focus first on #37121 and of course correctness goals. IOW we can have a widely used version that doesn't handle this yet, since most people work primarily at a leaf crate when building. But we should link this goal perhaps to some issues that pertain to specific impl changes for fixing it.
nikomatsakis commentedon Oct 21, 2016
triage: P-medium
add a test case for changing a private impl method across crates
nagisa commentedon Oct 22, 2016
The private callees might have been inlined through the public method.
KalitaAlexey commentedon Oct 23, 2016
@nagisa Good note. Is there any way to determine whether some private item was inlined? We must investigate this case.
michaelwoerister commentedon Oct 23, 2016
Inlining should mostly be handled by splitting interfaces from bodies for Metadata dep-nodes, like we also plan to do for HIR dep-nodes.
#[inline]then we should not need to care what got inlined into it.#[inline], we must track what goes into its body. If inlining happens at the MIR level, this should already be covered by the current tracking system. At LLVM level it's more difficult. I could imagine that we give the Rust compiler a heuristic that decides which functions are very unlikely to be inlined and then mark those functions with#[inline(never)]at the LLVM level --- and add dependency edges to all other callees because we can't guarantee that they haven't been inlined.hanna-kruppe commentedon Oct 23, 2016
Just because a function (in the same LLVM module) is
noinlinedoesn't mean LLVM won't peek at its implementation. For example, consider this playground example:The LLVM IR for
fiveisnoinline(in debug and in release), but in release mode,callergets optimized toret i32 6. It also adds various attributes tocallerthat can only be justified by analyzing or inliningfive(e.g.,nounwind).michaelwoerister commentedon Oct 23, 2016
@rkruppe Wow! That's a pretty blatant violation of what the documentation says about the attribute. Thanks for the info!
Oh well, we can still fall back to the brute force method of preventing inlining by moving those functions into their own compilation unit.
nagisa commentedon Oct 23, 2016
This optimisation happens because of global (cross-function) constant propagation and not inlining, so no violations here. The way I see it, this goal cannot be achieved without instrumenting LLVM appropriately.
As an aside, LLVM has quite a few module-wide optimisations (inlining and global constprop being two examples), all of which would have to be instrumented somehow. Separate compilation units sound quite inefficient to me at the first consideration.
eddyb commentedon Oct 23, 2016
Separate compilation units might work with ThinLTO, if we get the granularity just right.
michaelwoerister commentedon Oct 23, 2016
They are probably not ideal from a compile-time point of view.
One consequence of instrumenting LLVM would be that we have to push some of the dependency tracking past the LLVM passes. Probably not that big a deal.
A short-term solution would be to add dependency edges to everything that is transitively called within the same compilation unit (i.e. the worst case result that LLVM instrumentation data could yield).
6 remaining items
Rollup merge of rust-lang#67773 - michalt:issue-37333-test, r=nikomat…
Rollup merge of rust-lang#67773 - michalt:issue-37333-test, r=nikomat…
michalt commentedon Jan 8, 2020
With the test added, I think this can be closed now. :)
michaelwoerister commentedon Jan 9, 2020
Yes, the test looks good. Thanks, @michalt!