Remove the requirement that ast->hir lowering be reproducible by jseyfried · Pull Request #33296 · rust-lang/rust

jseyfried · 2016-04-30T20:02:45Z

This PR changes the ast->hir lowerer to be non-reproducible, and it removes the lowering context's id cache.

If the hir of an ast node needs to be reproduced, we can use the hir map instead of the lowerer -- for example, tcx.map.expect_expr(expr.id) instead of lower_expr(lcx, expr).

r? @nrc

nagisa · 2016-04-30T20:19:24Z

What’s the purpose of the change? Seems like a potentially plugin-breaking-change to me.

petrochenkov · 2016-04-30T20:21:39Z

Side note: if librustc_save_analysis doesn't use AST -> HIR lowering anymore, then MTWT tables can be cleared even if save_analysis is enabled. See #30145 (comment)

jseyfried · 2016-04-30T21:08:06Z

@nagisa

What’s the purpose of the change?

The purpose is to simplify lowering (by removing the id caching system) and to remove the lowering context from the CompileState.

Also, it will be significantly simpler to do name resolution during lowering if lowering doesn't need to be reproducible. I don't think it's worth the extra complexity needed to be reproducible when we can just use the HIR map to re-lower (almost) any AST node.

jseyfried · 2016-04-30T21:13:45Z

@petrochenkov good point, I added a commit that clears the MTWT tables after lowering for save-analysis.

nrc · 2016-05-01T21:49:52Z

I think you can remove this whole module

nrc · 2016-05-02T00:41:35Z

-// all increments being for lowering. This means that you should not call any
-// non-lowering function which will use new node ids.
-//
 // We must also cache gensym'ed Idents to ensure that we get the same Ident


This comment can probably go too

Good point, forgot about that -- done.

nrc · 2016-05-02T00:43:03Z

r+ with that comment removed

nrc · 2016-05-02T01:05:10Z

@bors: r+

bors · 2016-05-02T01:05:11Z

📌 Commit ef69ef8 has been approved by nrc

jseyfried · 2016-05-02T01:15:30Z

@nrc Thanks!
I'm almost done with a PR for resolving AST, pending #33190.

bors · 2016-05-02T01:28:24Z

⌛ Testing commit ef69ef8 with merge 855fb61...

@nrc

Remove the requirement that ast->hir lowering be reproducible This PR changes the ast->hir lowerer to be non-reproducible, and it removes the lowering context's id cache. If the `hir` of an `ast` node needs to be reproduced, we can use the hir map instead of the lowerer -- for example, `tcx.map.expect_expr(expr.id)` instead of `lower_expr(lcx, expr)`. r? @nrc

bors · 2016-05-02T03:39:32Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

RalfJung · 2016-05-06T13:07:50Z

What exactly is getting non-reproducible with this change? Does this mean that if I compile the same Rust program twice, I would get two different binaries (as in, binaries that are not bit-for-bit identical)? That would be a huge bummer -- reproducible builds are a cornerstone of building trustworthy binaries. If Rust can't build reproducibly, This is actively used, for example, by the Tor project and some BitCoin clients. it would be unsuited for any application that needs to establish trust in the generated binaries. See https://reproducible-builds.org/ for more details.

jseyfried · 2016-05-06T22:42:37Z

@RalfJung

Does this mean that if I compile the same Rust program twice, I would get two different binaries (as in, binaries that are not bit-for-bit identical)?

I don't know if Rust has this property, but this PR isn't related to reproducible builds. Lowering is still deterministic after this PR (i.e. it behaves the same when building the same code).

This PR changed lowering so that if the same AST node is lowered multiple times during the same build, we aren't guaranteed to get same result. Perhaps a better word would be "idempotent" instead of "reproducible".

@nrc

…nique, r=nrc Give `ast::ExprKind::Paren` no-op expressions the same ids as their children. Having `ast::ExprKind::Paren` expressions share ids with their children - reduces the number of unused `NodeId`s in the hir map and - guarantees that `tcx.map.expect_expr(ast_expr.id)` is the hir corresponding to `ast_expr`. This fixes the bug from rust-lang#34327, which was introduced in rust-lang#33296 when I assumed the above guarantee. r? @nrc

rust-highfive assigned nrc Apr 30, 2016

jseyfried force-pushed the non_idempotent_lowering branch 2 times, most recently from b1b28c5 to ce9e894Compare April 30, 2016 20:13

Panic on relowering an AST node with a cached node id
0649942

jseyfried force-pushed the non_idempotent_lowering branch from 0cc3573 to 864b4f8Compare April 30, 2016 21:35

jseyfried changed the title ~~Remove the requirement that ast->hir lowering be idempotent~~Remove the requirement that ast->hir lowering be reproducibleMay 1, 2016

nrc reviewed May 1, 2016
View reviewed changes

jseyfried added 5 commits May 1, 2016 23:30

Remove idempotent lowering test
02df9f3

Avoid using the lowering context in librustc_save_analysis
ca88c44

Remove the lowering context's id caching system
b8dc2a7

Cleanup formatting
298a1c0

Avoid keeping MTWT tables for save-analysis
522b6ed

jseyfried force-pushed the non_idempotent_lowering branch from 7a000ee to 522b6edCompare May 2, 2016 00:21

nrc reviewed May 2, 2016
View reviewed changes

Remove outdated comment
ef69ef8

bors merged commit ef69ef8 into rust-lang:masterMay 2, 2016

bors mentioned this pull request May 2, 2016
Refactor pretty printing to use the compiler API #33119
Merged

jseyfried deleted the non_idempotent_lowering branch May 7, 2016 06:34

jseyfried mentioned this pull request Jun 19, 2016
Give ast::ExprKind::Paren no-op expressions the same ids as their children. #34355
Merged

Uh oh!

Uh oh!

Conversation

jseyfried commented Apr 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagisa commented Apr 30, 2016

Uh oh!

petrochenkov commented Apr 30, 2016

Uh oh!

jseyfried commented Apr 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseyfried commented Apr 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrcMay 1, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfriedMay 2, 2016

Choose a reason for hiding this comment

Uh oh!

nrcMay 2, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfriedMay 2, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented May 2, 2016

Uh oh!

nrc commented May 2, 2016

Uh oh!

bors commented May 2, 2016

Uh oh!

jseyfried commented May 2, 2016

Uh oh!

bors commented May 2, 2016

Uh oh!

bors commented May 2, 2016

Uh oh!

RalfJung commented May 6, 2016

Uh oh!

jseyfried commented May 6, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jseyfried commented Apr 30, 2016•
edited
Loading

jseyfried commented Apr 30, 2016•
edited
Loading

jseyfried commented Apr 30, 2016•
edited
Loading

jseyfriedMay 2, 2016•
edited
Loading

jseyfried commented May 6, 2016•
edited
Loading