Stream: t-compiler

Topic: weekly meeting 2020-03-26 #54818


pnkfelix (Mar 26 2020 at 14:01, on Zulip):

Hi @T-compiler/meeting! Add a :wave: emoji to show you're here :)

pnkfelix (Mar 26 2020 at 14:02, on Zulip):

We'll open with five minutes for ...

pnkfelix (Mar 26 2020 at 14:02, on Zulip):

Announcements

pnkfelix (Mar 26 2020 at 14:02, on Zulip):

(agenda is here)

pnkfelix (Mar 26 2020 at 14:03, on Zulip):
pnkfelix (Mar 26 2020 at 14:04, on Zulip):

(there is some back and forth comments on the issue itself regarding whether the regular check of toolstate should be duty of T-release or T-compiler ... you can look over there and even chime in if you have something new to add to the discussion.)

Santiago Pastorino (Mar 26 2020 at 14:05, on Zulip):

just in case, I'm going to add that to T-compiler prioritization procedure

pnkfelix (Mar 26 2020 at 14:05, on Zulip):

but the summary is: for now it will remain our (T-compiler's) job; its just a question of how/when we look at toolstate breakage. Hopefully this new process will be more efficient than what we were doing before.

pnkfelix (Mar 26 2020 at 14:06, on Zulip):
pnkfelix (Mar 26 2020 at 14:07, on Zulip):

(I do hope that everyone is staying safe and maintaining their state of health in the face of all of this!!!)

pnkfelix (Mar 26 2020 at 14:09, on Zulip):

Okay, so, going through the agenda

pnkfelix (Mar 26 2020 at 14:09, on Zulip):

Next up we have four beta-nominations

pnkfelix (Mar 26 2020 at 14:09, on Zulip):

beta-nom 1/4: “Fix smaller issues with invalid placeholder type errors” #70369

centril (Mar 26 2020 at 14:10, on Zulip):

Not sure why this should be backported; it doesn't fix an ICE or anything

pnkfelix (Mar 26 2020 at 14:10, on Zulip):

@Esteban Küber was there a specific bug this was fixing?

pnkfelix (Mar 26 2020 at 14:11, on Zulip):

ah here is @Esteban Küber 's comment on the matter:

Nominating for backport (but with much lower priority) as I feel this goes in tandem with #70294.

pnkfelix (Mar 26 2020 at 14:11, on Zulip):

but yeah, without a specific reason, this is a :shrug: at best for me.

pnkfelix (Mar 26 2020 at 14:12, on Zulip):

@centril am I right that, in essence, this PR is the follow-up de-dup you referenced here ?

centril (Mar 26 2020 at 14:12, on Zulip):

@pnkfelix I think so

pnkfelix (Mar 26 2020 at 14:13, on Zulip):

(in which case, I'd say the plan of record was indeed to focus on back-porting just the #70294

centril (Mar 26 2020 at 14:13, on Zulip):

I would personally elect to backport #70294 but not the follow-up

pnkfelix (Mar 26 2020 at 14:13, on Zulip):

yeah, in hindsight these probably should have been listed so that #70294 came first

pnkfelix (Mar 26 2020 at 14:14, on Zulip):

okay, well I'm going to work with the assumption that we are declining to backport #70369

pnkfelix (Mar 26 2020 at 14:14, on Zulip):

beta-nom 2/4: “Account for bad placeholder types in where clauses” #70294

centril (Mar 26 2020 at 14:14, on Zulip):

(This one does fix an ICE)

pnkfelix (Mar 26 2020 at 14:15, on Zulip):

/me is loving how each one of these descriptions is saying "follow-up to PR xxx"

pnkfelix (Mar 26 2020 at 14:15, on Zulip):

(its like I'm traversing a linked list!)

pnkfelix (Mar 26 2020 at 14:16, on Zulip):

okay this looks like an easy beta-accept

pnkfelix (Mar 26 2020 at 14:16, on Zulip):

so, just for the record:

pnkfelix (Mar 26 2020 at 14:17, on Zulip):

#70369 declined for beta-backport

pnkfelix (Mar 26 2020 at 14:17, on Zulip):

#70294 beta-accepted

pnkfelix (Mar 26 2020 at 14:17, on Zulip):

beta-nom 3/4: “Fix ICE caused by truncating a negative ZST enum discriminant” #70126

centril (Mar 26 2020 at 14:18, on Zulip):

Well #70126 feels more risky, but on the other hand, stable-stable regression

pnkfelix (Mar 26 2020 at 14:19, on Zulip):

wait, "on the other hand" i usually take to mean "this second point overrides the first"

Wesley Wiser (Mar 26 2020 at 14:20, on Zulip):

This is my PR so I'm happy to answer questions if there are any.

pnkfelix (Mar 26 2020 at 14:20, on Zulip):

but often fixes to stable-to-stable regressions are things we can afford to let ride the trains

centril (Mar 26 2020 at 14:20, on Zulip):

@pnkfelix I think I'm going with "reluctantly beta-accepted" :slight_smile:

Wesley Wiser (Mar 26 2020 at 14:20, on Zulip):

I nominated because it's a stable-to-stable regression but I don't personally think this is super critical.

pnkfelix (Mar 26 2020 at 14:20, on Zulip):

/me goes to see when this was injected

pnkfelix (Mar 26 2020 at 14:20, on Zulip):

Rust 1.40

pnkfelix (Mar 26 2020 at 14:21, on Zulip):

My inclination is that we can let this ride the trains

nikomatsakis (Mar 26 2020 at 14:21, on Zulip):

I think I agree

pnkfelix (Mar 26 2020 at 14:21, on Zulip):

if it were injected in a more recent release, then I'd be more conflicted

pnkfelix (Mar 26 2020 at 14:21, on Zulip):

but the fact that this had laid fallow for this long, I take as a sign that its not a critical bug to justify a backport

centril (Mar 26 2020 at 14:21, on Zulip):

Seems reasonable

pnkfelix (Mar 26 2020 at 14:22, on Zulip):

okay

Wesley Wiser (Mar 26 2020 at 14:22, on Zulip):

The codegen bug has been there even longer IIRC

pnkfelix (Mar 26 2020 at 14:22, on Zulip):

#70126 declined for beta backport

pnkfelix (Mar 26 2020 at 14:22, on Zulip):

Wesley Wiser said:

The codegen bug has been there even longer IIRC

which is that referring to? The earlier bugs we approved for backports?

pnkfelix (Mar 26 2020 at 14:22, on Zulip):

or the one coming up?

Wesley Wiser (Mar 26 2020 at 14:23, on Zulip):

Sorry, there's two fixes in PR #70126

pnkfelix (Mar 26 2020 at 14:23, on Zulip):

ah right

pnkfelix (Mar 26 2020 at 14:23, on Zulip):

I did notice that its seemed like there were two only tangentially related changes in there. :slight_smile:

pnkfelix (Mar 26 2020 at 14:23, on Zulip):

I considered asking about it but then decided to let it slide

Wesley Wiser (Mar 26 2020 at 14:23, on Zulip):

I was just saying the codegen bug fixed in #70126 has been there for quite a few releases prior to 1.40 and no one has noticed.

pnkfelix (Mar 26 2020 at 14:23, on Zulip):

gotcha

pnkfelix (Mar 26 2020 at 14:24, on Zulip):

on the other hand, a codegen bug can be harder to notice than an ICE

pnkfelix (Mar 26 2020 at 14:24, on Zulip):

does that shift anyone's opinion here enough to push for a backport? I guess maybe @centril is implicitly saying we should rethink that?

centril (Mar 26 2020 at 14:25, on Zulip):

I'm comfortable with the overall "let it ride" :slight_smile:

pnkfelix (Mar 26 2020 at 14:25, on Zulip):

okay. lets just let it ride the trains

nikomatsakis (Mar 26 2020 at 14:26, on Zulip):

the bug report at least didn't seem to convey particular urgency

pnkfelix (Mar 26 2020 at 14:26, on Zulip):

beta-nom 4/4: “Ensure HAS_FREE_LOCAL_NAMES is set for ReFree” #69956

nikomatsakis (Mar 26 2020 at 14:26, on Zulip):

I'm not sure what exactly this fixes, but did the original PR make it into beta?

nikomatsakis (Mar 26 2020 at 14:27, on Zulip):

That is, I don't think there was any bug report in particular that prompted this fix, right @Matthew Jasper?

centril (Mar 26 2020 at 14:27, on Zulip):

Feels weird backporting something we don't know what it fixes :slight_smile:

pnkfelix (Mar 26 2020 at 14:28, on Zulip):

the main justification is that we know exactly when it was injected

Matthew Jasper (Mar 26 2020 at 14:28, on Zulip):

In principle it prevents incorrect use of global selection caches. I couldn't work out hope to use that to cause a bug.

pnkfelix (Mar 26 2020 at 14:28, on Zulip):

(in theory. though without a test, maybe it would have been buggy even before PR #69469 )

centril (Mar 26 2020 at 14:29, on Zulip):

@Matthew Jasper so in principle it could prevent a soundness hole?

pnkfelix (Mar 26 2020 at 14:30, on Zulip):

I am again inclined to let this ride the trains

Matthew Jasper (Mar 26 2020 at 14:30, on Zulip):

Maybe.

pnkfelix (Mar 26 2020 at 14:30, on Zulip):

but it is really hard to decide here

centril (Mar 26 2020 at 14:31, on Zulip):

I personally have no idea what the risk assessment here is wrt. correctness of the new impl vs. risk of soundness holes in the old

pnkfelix (Mar 26 2020 at 14:31, on Zulip):

I suppose this is the classic question of how to measure the risk introduced by this PR vs the risk of letting its bug go unfixed ...

centril (Mar 26 2020 at 14:32, on Zulip):

Maybe let's ask... @nikomatsakis @Matthew Jasper how confident are y'all in the correctness of the new impl?

pnkfelix (Mar 26 2020 at 14:32, on Zulip):

@Matthew Jasper so you just found this by reviewing your code and realizing you had mishandled this case?

nikomatsakis (Mar 26 2020 at 14:32, on Zulip):

/me reviews the PR briefly

Matthew Jasper (Mar 26 2020 at 14:32, on Zulip):

@pnkfelix Yes.

nikomatsakis (Mar 26 2020 at 14:34, on Zulip):

I'm pretty confident that this is correct (although in reviewing it I wonder if it's a bit overly conservative)

centril (Mar 26 2020 at 14:34, on Zulip):

conservative sounds good :slight_smile:

pnkfelix (Mar 26 2020 at 14:35, on Zulip):

i guess I'm willing to go along with a backport, since it followed the original PR by only 10 days or so

nikomatsakis (Mar 26 2020 at 14:35, on Zulip):

Yes, I think it's fine to backport

pnkfelix (Mar 26 2020 at 14:35, on Zulip):

so its largely an artifact of the train timing that we're having this conversation

centril (Mar 26 2020 at 14:35, on Zulip):

time check: 35 minutes in

pnkfelix (Mar 26 2020 at 14:35, on Zulip):

Okay.

pnkfelix (Mar 26 2020 at 14:35, on Zulip):

#69956 beta-accepted

pnkfelix (Mar 26 2020 at 14:36, on Zulip):

that's all the beta-nominations

pnkfelix (Mar 26 2020 at 14:36, on Zulip):

we have no stable nominations

pnkfelix (Mar 26 2020 at 14:36, on Zulip):

we have no PR's marked S-waiting-on-team.

Matthew Jasper (Mar 26 2020 at 14:36, on Zulip):

I'm confident that the PR is how the flags are intended to work. I think that any changes here should be to the selection caching code, not the flags.

pnkfelix (Mar 26 2020 at 14:36, on Zulip):

P-high Issues of Note

pnkfelix (Mar 26 2020 at 14:38, on Zulip):

For the stable-to-beta P-medium regressions, two are assigned (both to @Esteban Küber )

pnkfelix (Mar 26 2020 at 14:39, on Zulip):

the third, which is unassigned, is "no_mangle causes compilation errors with async-await on armv7-linux-androideabi and aarch64-linux-android targets" #70098

Esteban Küber (Mar 26 2020 at 14:39, on Zulip):

I'll try to get them done shortly, but I've been swamped lately

pnkfelix (Mar 26 2020 at 14:39, on Zulip):

which does have some active discussion, so that is good

pnkfelix (Mar 26 2020 at 14:39, on Zulip):

@Esteban Küber no worries

pnkfelix (Mar 26 2020 at 14:39, on Zulip):

I wasn't trying to call you out, at least in any negative way. :smiley:

pnkfelix (Mar 26 2020 at 14:40, on Zulip):

for the stable-to-nightly regressions, the P-high one is also unassigned. It is: "file not found for module" #70314

nikomatsakis (Mar 26 2020 at 14:41, on Zulip):

seems like that last one still needs minimization, but did we at least figure out what PR broke it?

pnkfelix (Mar 26 2020 at 14:41, on Zulip):

it looks like the cleanup crew is doing a great job over there trying to work with the bug filer to figure out what a reduced test would look like

centril (Mar 26 2020 at 14:41, on Zulip):

In case #70314 was injected by my expansion outline-module PR then I'm waiting for a minimal example to work with

centril (Mar 26 2020 at 14:42, on Zulip):

apparently it doesn't repro locally though

pnkfelix (Mar 26 2020 at 14:42, on Zulip):

anyway I think we can let discussion continue there as it currently is

centril (Mar 26 2020 at 14:42, on Zulip):

I did fix a similar-feeling issue

pnkfelix (Mar 26 2020 at 14:43, on Zulip):

okay, so, last (before WG checkins) is the nominated issues

pnkfelix (Mar 26 2020 at 14:43, on Zulip):

there are three on the agenda. (There's four according to github, but that's because one was nominated in the past few hours; we'll leave that one for next week's pre-triage)

pnkfelix (Mar 26 2020 at 14:44, on Zulip):

nom 1/3: “Compiler incorrectly assumes int will never be one” #69841

pnkfelix (Mar 26 2020 at 14:44, on Zulip):

Okay so this was discussed two weeks ago

centril (Mar 26 2020 at 14:44, on Zulip):

Namely, we have these LLVM-specific work items:

pnkfelix (Mar 26 2020 at 14:45, on Zulip):

there has been feedback from @Nikita Popov about how to respond

nikomatsakis (Mar 26 2020 at 14:46, on Zulip):

basically "excerpt this random set of hunks" from a patch

pnkfelix (Mar 26 2020 at 14:46, on Zulip):

so I think all we need is for someone to take charge here. (I think @Nikita Popov 's advice is that we should be able to doa backport of a narrow fix)

nikomatsakis (Mar 26 2020 at 14:46, on Zulip):

seems ok, if they have confidence that'll work :)

centril (Mar 26 2020 at 14:47, on Zulip):

would @Nikita Popov want to write up the PR perhaps?

pnkfelix (Mar 26 2020 at 14:47, on Zulip):

note that this was injected quite some time ago

pnkfelix (Mar 26 2020 at 14:47, on Zulip):

which to me means we don't need to put super high priority on a fix. I.e., I think we'd be happy with a fix suitable for riding the trains; it doesn't need to be tailored into something that is beta-backportable, in my opinion.

pnkfelix (Mar 26 2020 at 14:48, on Zulip):

(which means maybe we could mentor someone on it?)

nikomatsakis (Mar 26 2020 at 14:48, on Zulip):

seems fine. I guess the other question is status of a more general LLVM upgrade

pnkfelix (Mar 26 2020 at 14:48, on Zulip):

Oh that is true

pnkfelix (Mar 26 2020 at 14:48, on Zulip):

@simulacrum where are we on the more general LLVM upgrade at this point?

centril (Mar 26 2020 at 14:48, on Zulip):

I believe prep work is being done for LLVM 10

nikomatsakis (Mar 26 2020 at 14:48, on Zulip):

(If we did backport, I think the steps required are to excerpt the patch and apply to the rust-llvm repo and then bump our revision locally?)

pnkfelix (Mar 26 2020 at 14:49, on Zulip):

Okay given that there's no one chomping at the bit here to take this on

pnkfelix (Mar 26 2020 at 14:49, on Zulip):

I'm going to self-assign it as a mentor

centril (Mar 26 2020 at 14:49, on Zulip):

centril said:

I believe prep work is being done for LLVM 10

Specifically, https://github.com/rust-lang/rust/pull/70163

simulacrum (Mar 26 2020 at 14:49, on Zulip):

We're stalled on LLVM

simulacrum (Mar 26 2020 at 14:49, on Zulip):

(sizeable perf regression, unknown causes)

pnkfelix (Mar 26 2020 at 14:50, on Zulip):

(or maybe just mark it as a E-mentor bug? the mentor's don't self-assign ... unless they're self-assigning to indicate that they need to write up mentorship instructions, maybe?)

pnkfelix (Mar 26 2020 at 14:50, on Zulip):

(maybe mentors should self-assign?)

nikomatsakis (Mar 26 2020 at 14:50, on Zulip):

@pnkfelix I would say write-up some instructions and let's tweet out or something

pnkfelix (Mar 26 2020 at 14:50, on Zulip):

@nikomatsakis yeah sounds great

pnkfelix (Mar 26 2020 at 14:51, on Zulip):

moving on

nikomatsakis (Mar 26 2020 at 14:51, on Zulip):

I usually self assign only for the time to write up instructions

pnkfelix (Mar 26 2020 at 14:51, on Zulip):

nom: “Add cryptographic hash of source files in debug info” #69718

centril (Mar 26 2020 at 14:51, on Zulip):

simulacrum said:

(sizeable perf regression, unknown causes)

blargh... :frown: -- That cranelift backend will come in handy :slight_smile:

nikomatsakis (Mar 26 2020 at 14:52, on Zulip):

Ah, yes. So #69718 -- this is adding source files hash into debug info -- you can find my summary comment here

nikomatsakis (Mar 26 2020 at 14:52, on Zulip):

I wanted to raise this and "give notice" about the intent to land, following the (not yet fully formalized) MCP procedure

pnkfelix (Mar 26 2020 at 14:53, on Zulip):

what are the risks here?

pnkfelix (Mar 26 2020 at 14:53, on Zulip):

oh okay, its "just" a major change proposal?

nikomatsakis (Mar 26 2020 at 14:53, on Zulip):

I don't really think there are risks

pnkfelix (Mar 26 2020 at 14:53, on Zulip):

sounds great

nikomatsakis (Mar 26 2020 at 14:53, on Zulip):

But it's true that we'd probably want to continue doing this

nikomatsakis (Mar 26 2020 at 14:53, on Zulip):

Though we don't really "guarantee" much about the status of our debuginfo per se

pnkfelix (Mar 26 2020 at 14:53, on Zulip):

continuing doing which?

nikomatsakis (Mar 26 2020 at 14:53, on Zulip):

That said, other compiles do it, and I don't know that there's a reason not to include it

nikomatsakis (Mar 26 2020 at 14:53, on Zulip):

Continue including hashes of source files in debuginfo

pnkfelix (Mar 26 2020 at 14:54, on Zulip):

as in, follow through on this PR?

pnkfelix (Mar 26 2020 at 14:54, on Zulip):

"continue" implies that we were doing it, which, I infer, we were not?

nikomatsakis (Mar 26 2020 at 14:54, on Zulip):

Right, like if we reverted it, we might disrupt people's workflows

nikomatsakis (Mar 26 2020 at 14:54, on Zulip):

We do not currently include file hash information

pnkfelix (Mar 26 2020 at 14:54, on Zulip):

Okay, I understand, i think.

pnkfelix (Mar 26 2020 at 14:55, on Zulip):

/me prefers "Commit to doing this" phrasing

nikomatsakis (Mar 26 2020 at 14:55, on Zulip):

But I don't know why we would want to stop including hashes

pnkfelix (Mar 26 2020 at 14:55, on Zulip):

okay next up

pnkfelix (Mar 26 2020 at 14:55, on Zulip):

nom: “Box<dyn FnOnce> doesn’t respect self alignment” #68304

pnkfelix (Mar 26 2020 at 14:56, on Zulip):

@eddyb has written a summary of their proposal

bjorn3 (Mar 26 2020 at 14:56, on Zulip):

It would be really useful for cg_clif to implement that proposal

centril (Mar 26 2020 at 14:56, on Zulip):

We can also make this pattern be gated by a separate feature-gate (rather than unsized_locals), and make sure the standard library only uses that feature-gate and not unsized_locals.

:+1: -- I like the caution here.

pnkfelix (Mar 26 2020 at 14:57, on Zulip):

I think we can let follow-up discussion on #68304 move forward on either a Zulip topic, or on the issue itself

pnkfelix (Mar 26 2020 at 14:57, on Zulip):

Lets hear from @WG-learning ?

bjorn3 (Mar 26 2020 at 14:57, on Zulip):

bjorn3 said:

It would be really useful for cg_clif to implement that proposal

That would make me able to remove https://github.com/bjorn3/rustc_codegen_cranelift/blob/b113e88ddb399f92f0d044caa35e668d2499bdd9/src/base.rs#L67-L147

mark-i-m (Mar 26 2020 at 14:57, on Zulip):

Hi (for WG learning)!

mark-i-m (Mar 26 2020 at 14:58, on Zulip):

Next steps:

mark-i-m (Mar 26 2020 at 14:59, on Zulip):

Generally, I think it has been a productive time for us :)

pnkfelix (Mar 26 2020 at 14:59, on Zulip):

Great

nikomatsakis (Mar 26 2020 at 14:59, on Zulip):

:+1: to renaming -- I think some of our existing working groups (cough #wg-traits cough) have somewhat overly broad names, and that tailoring names will help clarify goals

nikomatsakis (Mar 26 2020 at 14:59, on Zulip):

Also, this is awesome work

pnkfelix (Mar 26 2020 at 15:00, on Zulip):

I am going to assume that there is nothing to report from @WG-llvm . (It should probably be recast as something other than a WG, but that's a discussion for another day)

pnkfelix (Mar 26 2020 at 15:00, on Zulip):

also, I need to sign off for another meeting

pnkfelix (Mar 26 2020 at 15:00, on Zulip):

so, thanks once again to everyone in @T-compiler/meeting for attending! you all are awesome! Stay healthy! stay figuratively close and literally far far away!

nikomatsakis (Mar 26 2020 at 15:01, on Zulip):

I have to sign off too but I want to post one final announcement I forgot:

nikomatsakis (Mar 26 2020 at 15:02, on Zulip):

(also, @Santiago Pastorino and I started work on an RFC formalizing the "major change" process, if you're confused, the key point of it is that you write-up a change, find someone to "second" it, and we have a lightweight "give notice" period and then go ahead and do it)

nikomatsakis (Mar 26 2020 at 15:02, on Zulip):

(but I'm trying to unblock some of the stuff that was opened in the meantime:)

Nikita Popov (Mar 26 2020 at 15:05, on Zulip):

Yeah, WG-llvm not doing much :) LLVM 10 upgrade is stuck on compile-time performance issues. I would appreciate it if a second pair of eyes could grab some CI artifacts from https://github.com/rust-lang/rust/pull/67759 and check if they see something I don't. I've also set up http://llvm-compile-time-tracker.com/graphs.php to avoid more compile-time regressions in LLVM in the future, will see how that goes.

Last update: May 29 2020 at 17:55UTC