Prevent fmt::Arguments from being shared across threads by oli-obk · Pull Request #45198 · rust-lang/rust

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

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

Fixesrust-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 Fixesrust-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
Fixesrust-lang#45197

oli-obk force-pushed the fmt_args branch from dcc8458 to 787f9f4Compare 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 [DEPRECATED; DO NOT USE] 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 label Oct 30, 2017

kennytm added the S-waiting-on-team label Nov 14, 2017

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. S-waiting-on-team labels Nov 22, 2017

bors merged commit dc7de37 into rust-lang:masterNov 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

Uh oh!

Uh oh!

Conversation

oli-obk commented Oct 11, 2017

Uh oh!

arielb1 commented Oct 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Oct 11, 2017

Uh oh!

arielb1Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

arielb1Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

oli-obkOct 11, 2017

Choose a reason for hiding this comment

Uh oh!

oli-obkOct 11, 2017

Choose a reason for hiding this comment

Uh oh!

oli-obkOct 11, 2017

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Oct 11, 2017

Uh oh!

bors commented Oct 11, 2017

Uh oh!

arielb1 commented Oct 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1 commented Oct 11, 2017

Uh oh!

bors commented Oct 11, 2017

Uh oh!

arielb1 commented Oct 11, 2017

Uh oh!

ghost commented Oct 11, 2017

Uh oh!

Mark-Simulacrum commented Oct 11, 2017

Uh oh!

Mark-Simulacrum commented Oct 11, 2017

Uh oh!

alexcrichton commented Oct 11, 2017

Uh oh!

sfackler commented Oct 11, 2017

Uh oh!

arielb1 commented Oct 11, 2017

Uh oh!

cuviper commented Oct 11, 2017

Uh oh!

arielb1 commented Oct 11, 2017

Uh oh!

Mark-Simulacrum commented Oct 11, 2017

Uh oh!

bors commented Oct 11, 2017

Uh oh!

kennytm commented Oct 28, 2017

Uh oh!

alexcrichton commented Oct 31, 2017

Uh oh!

alexcrichton commented Oct 31, 2017

Uh oh!

aidanhs commented Nov 3, 2017

Uh oh!

cuviper commented Nov 8, 2017

Uh oh!

Mark-Simulacrum commented Nov 8, 2017

Uh oh!

cuviper commented Nov 8, 2017

Uh oh!

dpc commented Nov 8, 2017

Uh oh!

carols10cents commented Nov 13, 2017

Uh oh!

Mark-Simulacrum commented Nov 13, 2017

Uh oh!

kennytm commented Nov 13, 2017

Uh oh!

arielb1 commented Oct 11, 2017•
edited
Loading

arielb1 commented Oct 11, 2017•
edited
Loading