-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some data is not hashed by the SVH that ought to be #32753
Comments
Having some sort of hash match for transitive dependencies seems like it may still be important for the time being? I don't think there's basically any use case where you can recompile an upstream dependency without recompiling intermediate ones and expect it to actually work robustly, so this is largely just preventing us from shooting ourselves in the foot. In any case, changing the SVH to ICH seems fine by me, I think they'd both satisfy the only remaining purpose I know of for SVH at least. |
A proper ICH implementation would probably be a better SVH than the current one, so I'm for re-using the ICH as SVH in the long run. I would keep the SVH around for now and then do the switch when the ICH implementation seems complete enough. Regarding debuginfo: It should be fine to just use the crate-disambiguator instead of the SVH there. I'll make a PR changing that later this week. |
OK, I just took a closer look at the current SVH implementation, gauging its suitability for being used as ICH. My assessment is that we are almost there. Here are the specific points that need to be addressed from my point of view:
Regarding name resolution, consider the following case: mod mod1 {
struct Foo(u32);
}
mod mod2 {
struct Foo(i64);
}
mod mod3 {
use mod1::Foo;
fn bar() -> usize {
mem::size_of::<Foo>()
}
} Using the current algorithm, neither the hash of One possible solution to this would be: For each In the above example, we would then have fed |
I'm going to take a stab at hacking on this. |
Somewhat surprisingly, I'm finding that the hash of items where the only change is a |
Also, it seems likely that we don't need to include nested items in the hash at all, no? |
I would think so too. |
Never mind, my test was bogus. |
I edited the summary to incorporate @michaelwoerister's analysis results as a check-list of changes. |
Note: It's probably a good idea to add a |
Various improvements to the SVH This fixes a few points for the SVH: - incorporate resolve results into the SVH; - don't include nested items. r? @michaelwoerister cc #32753 (not fully fixed I don't think)
…atsakis incr. comp.: Take spans into account for ICH This PR makes the ICH (incr. comp. hash) take spans into account when debuginfo is enabled. A side-effect of this is that the SVH (which is based on the ICHs of all items in the crate) becomes sensitive to the tiniest change in a code base if debuginfo is enabled. Since we are not trying to model ABI compatibility via the SVH anymore (this is done via the crate disambiguator now), this should be not be a problem. Fixes #33888. Fixes #32753.
New summary:
I think we've all agreed by now that we will focus on the ICH (incremental compilation hash) and revisit the question of a SVH (stable version hash, used to detect ABI-incompatible changes) at some future date. The "original summary" (below) goes into some of the discussion.
I've thus repurposed this issue to target addressing the shortcomings of the existing hash for the purposes of incremental compilation. The following checklist is derived from @michaelwoerister's comment:
visit_stmt()
andvisit_decl()
by themselves should not change the hash, since they could just represent a nested item (which we either want to ignore completely or handle in a position independent way).CaptureClause
of closures should not be ignoredwalk_generics()
invisit_variant()
andvisit_variant_data()
are redundant; this is already handled byvisit_item()
ExprLoop
,ExprBreak
, andExprAgain
is redundant,visit_name()
is called for those anyway (as already done forExprWhile
)There are also optional refinements:
Original summary:
In #32016, I (ab)used the SVH to serve as a hash for the purposes of incremental compilation. This is simply wrong, because the SVH intentionally omits some details that do not affect the ABI, but which very much affect whether code needs to be recompiled (e.g., the value of a constant). This needs to be fixed, but in one of two ways:
At this point, there are really very few users of the SVH:
The second use is unnecessary. The first use is interesting. It's unclear how flexible this check is. But if we used the ICH for this purpose, it would basically be equivalent to the SVH today (the SVH today is looser, as I wrote in the beginning, but effectively any change will cause the SVH to change, with very few exceptions).
Moreover, I claim that even if, someday, we wanted to modify the check to be true ABI compatibility, we probably wouldn't want a single SVH hash. We'd probably want each function hash to include information about the way the argument types are represented, so that we can detect mismatches on a fine-grained level, rather than just "some fn, which you may not even call, changed". But I'm not sure.
One problem with using the ICH everywhere, though, is that we must be careful about endianness. We'd prefer if cross-compiling did not affect the ICH.
I guess I've wasted more time writing this paragraph than it really merits. End of the day, the choice is:
cc @michaelwoerister
The text was updated successfully, but these errors were encountered: