Prevent fmt::Arguments from being shared across threads #45198

oli-obk · 2017-10-11T09:01:07Z

This is a breaking change! Without doing this it's very easy to create race conditions.

There's probably a way to do this without breaking valid use cases, but it would require quite an overhaul of the formatting machinery.

arielb1 · 2017-10-11T09:36:53Z

I would prefer to put a PhantomData<*mut TraitObject> type there (e.g. PhantomData<*mut Fn()>) - that would prohibit all OIBITs.

oli-obk · 2017-10-11T11:12:55Z

Done

r? @arielb1

arielb1 · 2017-10-11T11:14:55Z

src/test/run-pass-fulldeps/proc-macro/span-api-tests.rs

@@ -12,6 +12,7 @@
 // aux-build:span-test-macros.rs

 // ignore-pretty
+// ignore-stage1


why is that?

why are you changing a test that should be unaffected? Is this only for the snapshot? In that case, I think you should add a SNAP: remove after snapshot comment.

I've been testing the fmt::Arguments changes on stage 1, but it keeps failing on this test even without my changes. Someone forgot to add the flag, which is required for most proc-macro tests.

I don't think it has anything to do with snapshots, just with compiler plugins. I can remove this comment from this PR. It's just an annoyance I encountered

removed and squashed

Fixes rust-lang#45197

arielb1 · 2017-10-11T12:06:39Z

@bors r+

bors · 2017-10-11T12:06:40Z

📌 Commit 787f9f4 has been approved by arielb1

arielb1 · 2017-10-11T12:07:20Z

Umm I would actually rather like a comment on _oibit_remover explaining its purpose.

@bors r-

arielb1 · 2017-10-11T13:04:35Z

@bors r+

bors · 2017-10-11T13:04:35Z

📌 Commit dc7de37 has been approved by arielb1

arielb1 · 2017-10-11T13:04:41Z

@bors rollup

ghost · 2017-10-11T14:18:35Z

@arielb1
Out of curiousity, is there any difference between PhantomData<*mut u8> and PhantomData<*mut Fn()>?

Prevent fmt::Arguments from being shared across threads Fixes rust-lang#45197 This is a **breaking change**! Without doing this it's very easy to create race conditions. There's probably a way to do this without breaking valid use cases, but it would require quite an overhaul of the formatting machinery.

Mark-Simulacrum · 2017-10-11T16:59:23Z

@bors r-

Mark-Simulacrum · 2017-10-11T17:00:32Z

@rust-lang/libs -- nominating for discussion, I think this (as a breaking change) deserves some discussion before merging.

alexcrichton · 2017-10-11T17:34:52Z

Ideally we'd have a crater run to evaluate the impact here, but on the other hand it's not like we're not going to fix this. Rather, we may fix it more slowly if this ends up having a lot of fallout.

In any case I'd personally be ok landing this as-is as it's on nightly and we can always revert it if the problem is too widespread.

sfackler · 2017-10-11T18:11:01Z

I agree. It seems pretty unlikely to me that anyone is writing code that depends on the Send or Sync-ness here.

arielb1 · 2017-10-11T18:11:48Z

@arielb1
Out of curiousity, is there any difference between PhantomData<*mut u8> and PhantomData<*mut Fn()>?

Sure. Only the latter prevents OIBITs.

cuviper · 2017-10-11T18:29:40Z

@arielb1
Out of curiousity, is there any difference between PhantomData<*mut u8> and PhantomData<*mut Fn()>?

Sure. Only the latter prevents OIBITs.

Whereas PhantomData<*mut u8> only prevents Send and Sync in particular?

arielb1 · 2017-10-11T19:08:39Z

Whereas PhantomData<*mut u8> only prevents Send and Sync in particular?

It "accidentally" prevents Send and Sync becuase *mut T is neither of these.

Mark-Simulacrum · 2017-10-11T21:58:46Z

@bors try

Well, I'll start a try build so that we can get a cargobomb run complete sooner should we decide to go that route.

I don't think we should merge without results from that, but am fine with being overruled.

bors · 2017-10-11T21:58:56Z

⌛ Trying commit dc7de37 with merge e7aa529cd92736ab0f7400ad33835c5f64e93fc7...

kennytm · 2017-10-28T20:26:15Z

@oli-obk Two PRs are pending at the moment (#45333, #45205), one is queued (#45225).

Since there are no new commits, do we reuse the existing try build or generate a new one?

alexcrichton · 2017-10-31T22:15:51Z

@aidanhs just checking in, is this unblocked now to get another crater run?

alexcrichton · 2017-10-31T22:19:06Z

@kennytm oh to answer your question I think it's ok to use the same artifacts from before.

aidanhs · 2017-11-03T02:20:02Z

Crater run started.

cuviper · 2017-11-08T01:00:09Z

@aidanhs Did crater finish this?

Mark-Simulacrum · 2017-11-08T15:20:33Z

Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-45198-2/index.html

cuviper · 2017-11-08T18:04:48Z

@dpc it looks like the slog fix was merged into the 1.5.0 branch, but not the 1.x.y branch from which you released slog-1.6.0.

dpc · 2017-11-08T21:04:31Z

Oh my. Sorry. I've added it to the 1.x.y branch: https://github.com/slog-rs/slog/commits/1.x.y and I've published 1.7.1 with another nightly fix and since this is passing: https://travis-ci.org/slog-rs/slog/builds/299306472 I think we're good now. Please double check.

carols10cents · 2017-11-13T17:16:49Z

@aidanhs @Mark-Simulacrum looks like this might need another crater run?

Mark-Simulacrum · 2017-11-13T18:25:58Z

It'll probably be at least 8-10 days before Crater is ready for this PR -- we have 2 in queue right now. If the last Crater run only recorded problems due to slog failing, them I'm somewhat inclined to just push this through -- @rust-lang/libs: What do you think? We'll run crater on beta anyway, so I'm not too worried...

kennytm · 2017-11-13T18:32:17Z

Could we just run a "mini-crater" test on those 62 regressed crates?

Mark-Simulacrum · 2017-11-13T18:39:41Z

Seems possible, not sure -- @aidanhs may be able to comment on that, I don't know crater internals well enough to.

sfackler · 2017-11-21T22:53:37Z

It has now been over a month. I am going to r+ this thing in an hour unless someone strenuously objects.

Mark-Simulacrum · 2017-11-22T00:59:29Z

@bors r=sfackler

Yeah, we should've pushed this through faster. I believe there are no significant remaining concerns, and beta just branched, so I'm happy to let this bake on nightly.

bors · 2017-11-22T00:59:30Z

📌 Commit dc7de37 has been approved by sfackler

kennytm · 2017-11-22T11:58:51Z

@bors rollup-

Better not to roll this up given the potential breakage.

bors · 2017-11-22T12:35:02Z

⌛ Testing commit dc7de37 with merge 1dc0b57...

Prevent fmt::Arguments from being shared across threads Fixes #45197 This is a **breaking change**! Without doing this it's very easy to create race conditions. There's probably a way to do this without breaking valid use cases, but it would require quite an overhaul of the formatting machinery.

bors · 2017-11-22T15:08:12Z

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 1dc0b57 to master...

rust-highfive assigned arielb1 Oct 11, 2017

arielb1 reviewed Oct 11, 2017

View reviewed changes

Prevent fmt::Arguments from being shared across threads

787f9f4

Fixes rust-lang#45197

oli-obk force-pushed the fmt_args branch from dcc8458 to 787f9f4 Compare October 11, 2017 11:26

Explain the _oibit_remover field

dc7de37

kennytm mentioned this pull request Oct 11, 2017

Rollup of 9 pull requests #45204

Closed

Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 11, 2017

Mark-Simulacrum added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 11, 2017

kennytm added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Oct 28, 2017

carols10cents removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Oct 30, 2017

kennytm added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Nov 14, 2017

bors merged commit dc7de37 into rust-lang:master Nov 22, 2017

dtolnay mentioned this pull request Jan 26, 2018

Old version (1.x) of the slog crate can't compile #47781

Closed

jebrosen mentioned this pull request Sep 24, 2019

Spurious new Send requirement in async block #64477

Closed

oli-obk deleted the fmt_args branch June 15, 2020 15:26

bjorn3 mentioned this pull request Dec 16, 2020

Misc cleanups ykjit/yk#194

Merged

Prevent fmt::Arguments from being shared across threads #45198

Prevent fmt::Arguments from being shared across threads #45198

Conversation

oli-obk commented Oct 11, 2017

arielb1 commented Oct 11, 2017 • edited Loading

oli-obk commented Oct 11, 2017

arielb1 Oct 11, 2017

Choose a reason for hiding this comment

arielb1 Oct 11, 2017

Choose a reason for hiding this comment

oli-obk Oct 11, 2017

Choose a reason for hiding this comment

oli-obk Oct 11, 2017

Choose a reason for hiding this comment

oli-obk Oct 11, 2017

Choose a reason for hiding this comment

arielb1 commented Oct 11, 2017

bors commented Oct 11, 2017

arielb1 commented Oct 11, 2017 • edited Loading

arielb1 commented Oct 11, 2017

bors commented Oct 11, 2017

arielb1 commented Oct 11, 2017

ghost commented Oct 11, 2017

Mark-Simulacrum commented Oct 11, 2017

Mark-Simulacrum commented Oct 11, 2017

alexcrichton commented Oct 11, 2017

sfackler commented Oct 11, 2017

arielb1 commented Oct 11, 2017

cuviper commented Oct 11, 2017

arielb1 commented Oct 11, 2017

Mark-Simulacrum commented Oct 11, 2017

bors commented Oct 11, 2017

kennytm commented Oct 28, 2017

alexcrichton commented Oct 31, 2017

alexcrichton commented Oct 31, 2017

aidanhs commented Nov 3, 2017

cuviper commented Nov 8, 2017

Mark-Simulacrum commented Nov 8, 2017

cuviper commented Nov 8, 2017

dpc commented Nov 8, 2017

carols10cents commented Nov 13, 2017

Mark-Simulacrum commented Nov 13, 2017

kennytm commented Nov 13, 2017

Mark-Simulacrum commented Nov 13, 2017

sfackler commented Nov 21, 2017

Mark-Simulacrum commented Nov 22, 2017

bors commented Nov 22, 2017

kennytm commented Nov 22, 2017

bors commented Nov 22, 2017

bors commented Nov 22, 2017

arielb1 commented Oct 11, 2017 •

edited

Loading

arielb1 commented Oct 11, 2017 •

edited

Loading