Stream: t-compiler/wg-nll

Topic: polonius-graphviz


Andrea Lattuada (May 28 2018 at 11:47, on Zulip):

@nikomatsakis checking our understanding of the issue (graphviz https://github.com/rust-lang-nursery/polonius/issues/12):
- the output dump (when run with -v) should already contain all the info we need (but we’re still a bit fuzzy on the details here);
- the graph structure is pre-determined by the cfg_edges;
- we need to combine the two.

nikomatsakis (May 28 2018 at 12:12, on Zulip):

@Andrea Lattuada this sounds roughly right, yes

nikomatsakis (May 28 2018 at 12:12, on Zulip):

basically the cfg-edge gives the structure of the graph, which would mirrow the input control-flow graph

nikomatsakis (May 28 2018 at 12:12, on Zulip):

but we can then display the input/output facts at each point

Yati Sagade (May 28 2018 at 12:22, on Zulip):

++ thanks for the clarification

Andrea Lattuada (May 28 2018 at 14:31, on Zulip):

Current graphviz output (with only borrow_regions as inputs): graph.svg

Andrea Lattuada (May 29 2018 at 08:00, on Zulip):

Hi @Yati Sagade, I’m going to try and add support for the other input/output collections (probably via a Trait / the OutputDump trait)

Yati Sagade (May 29 2018 at 08:21, on Zulip):

Hey! sure, let me know when you push something to the branch.

Yati Sagade (May 29 2018 at 08:24, on Zulip):

@Andrea Lattuada The stuff currently being plotted, are they not just inputs? I see there's one borrow_live_at member in polonius_engine::facts::Output. Since that is the thing that is printed out it seems that contains the outputs per point.

Andrea Lattuada (May 29 2018 at 08:34, on Zulip):

@Yati Sagade Yes, and only for borrow_region.

Andrea Lattuada (May 29 2018 at 08:34, on Zulip):

I just pushed a fix for the formatting.

Andrea Lattuada (May 29 2018 at 08:34, on Zulip):

I think you’re right, we need to look into Output for the outputs per point, possibly in a similar way as what we’re doing for inputs.

Andrea Lattuada (May 29 2018 at 08:36, on Zulip):

I’m writing a generic function that should be able to handle many/all of the tables (input and output), almost done.

Yati Sagade (May 29 2018 at 08:38, on Zulip):

++

Andrea Lattuada (May 29 2018 at 08:54, on Zulip):

@Yati Sagade pushed the new function: I have changed some things around so it may need some explanation.

Yati Sagade (May 29 2018 at 08:56, on Zulip):

Checking now

Andrea Lattuada (May 29 2018 at 08:58, on Zulip):

The meat is facts_by_point, which formats facts from a certain table (input/output), given:

Yati Sagade (May 29 2018 at 09:05, on Zulip):

I've pushed a small patch to name the boxes after the points

Andrea Lattuada (May 29 2018 at 09:06, on Zulip):

Cool, thanks! (Sorry, I think I broke that while changing things)

Andrea Lattuada (May 29 2018 at 09:07, on Zulip):

Does the rest make sense?

Yati Sagade (May 29 2018 at 09:13, on Zulip):

No it was using indexes before. I think I get it now.
Now we need to do inject the other relations in to inputs_by_point, and then figure out outputs. Right?

Andrea Lattuada (May 29 2018 at 09:16, on Zulip):

Yup! I can take care of injecting the other inputs. Are you up for taking a look at how to work on outputs?

Yati Sagade (May 29 2018 at 09:17, on Zulip):

Also I was wondering if it would make sense to have each relation even if there was nothing in that set at a given point, instead of the hyphen (or just not have the empty boxes with hyphens at all where applicable).

Yati Sagade (May 29 2018 at 09:17, on Zulip):

Yes, I can play with the outputs

Andrea Lattuada (May 29 2018 at 09:27, on Zulip):

Also I was wondering if it would make sense to have each relation even if there was nothing in that set at a given point, instead of the hyphen (or just not have the empty boxes with hyphens at all where applicable).

We should probably hide them, yes. I was mostly being lazy with the logic there.

Andrea Lattuada (May 29 2018 at 09:53, on Zulip):

Pushed all inputs, and removed empty cells.

Yati Sagade (May 29 2018 at 09:53, on Zulip):

I added the borrow_live_at output: Does this make sense? graph.svg

Yati Sagade (May 29 2018 at 09:54, on Zulip):

(will push the code if you think this is ok)

Andrea Lattuada (May 29 2018 at 09:56, on Zulip):

Yup! Looks good to me!

Yati Sagade (May 29 2018 at 09:57, on Zulip):

++ pushing now

Yati Sagade (May 29 2018 at 10:06, on Zulip):

weird merge conflict, resolving and then pushing

Yati Sagade (May 29 2018 at 10:10, on Zulip):

Done. Produces something like that: graph.svg

Yati Sagade (May 29 2018 at 10:12, on Zulip):

Did I mess up something you intended to have? I see that you had a flat_map on inputs_by_point, which I changed to a filter_map so we don't have spurious boxes.

Andrea Lattuada (May 29 2018 at 10:17, on Zulip):

Nice!

Andrea Lattuada (May 29 2018 at 10:18, on Zulip):

No, looks good. I think we made the same change in two slightly different ways.

Yati Sagade (May 29 2018 at 10:20, on Zulip):

I'll add the other outputs now

Andrea Lattuada (May 29 2018 at 10:20, on Zulip):

Could probably still use some refinement, but I think this is probably almost good to ship. @nikomatsakis opinions? (the latest render posted by @Yati Sagade is graph.svg )

Andrea Lattuada (May 29 2018 at 10:21, on Zulip):

I'll add the other outputs now

Ah, right.

Yati Sagade (May 29 2018 at 10:42, on Zulip):

Ok pushed it. Let me know if you think that macro is a pain, and I'll just inline it

Yati Sagade (May 29 2018 at 10:43, on Zulip):

@nikomatsakis, @Andrea Lattuada graph.svg

qmx (May 29 2018 at 10:44, on Zulip):

that's beatiful!

Andrea Lattuada (May 29 2018 at 10:46, on Zulip):

Ok pushed it. Let me know if you think that macro is a pain, and I'll just inline it

Cool! Let me do a bit of cleanup (I think we can avoid the loop by accepting any Iterator<F> in facts_by_point.

nikomatsakis (May 29 2018 at 10:51, on Zulip):

looking good! one nit: region_live_at is presently an input, not an output

Yati Sagade (May 29 2018 at 11:00, on Zulip):

Ah right, removed it.

Yati Sagade (May 29 2018 at 11:01, on Zulip):

@Andrea Lattuada cool

Andrea Lattuada (May 29 2018 at 11:14, on Zulip):

@Andrea Lattuada cool

Pushed.

Andrea Lattuada (May 29 2018 at 11:16, on Zulip):

looking good! one nit: region_live_at is presently an input, not an output

@nikomatsakis The region_live_ats are expanded during evaluation, though, right? i.e. there will be more at the end: don’t we want to show those?

Yati Sagade (May 29 2018 at 11:20, on Zulip):

Ah yes indeed, I see it is expanded after the analysis

Yati Sagade (May 29 2018 at 11:25, on Zulip):

@Janito Vaqueiro Ferreira Filho https://github.com/utaal/polonius branch graphviz-output
Example run: cargo +nightly run --release -- --graphviz_file=graph.dot inputs/issue-47680/nll-facts/main. To generate the graph, either paste it on an online graphviz tool, or if you have dot installed, dot -o graph.svg -Tsvg graph.dot

lqd (May 29 2018 at 11:39, on Zulip):

@Andrea Lattuada what do you mean expanded during evaluation ? IIRC there won't be new region_live_at produced by the computation; are you specifically talking about the bit of code that adds them to the output in "-v/dump_enabled" mode ? If so, I think this is more about dumping "more information" than it being absolutely required to be shown :)

nikomatsakis (May 29 2018 at 12:17, on Zulip):

I think they mean the way that we add the universal regions to region_live_at — that's only a temp hack, but it's true that it is presently modified

Yati Sagade (May 29 2018 at 12:29, on Zulip):

@nikomatsakis @Andrea Lattuada So as it stands now, is it good to go to master?

nikomatsakis (May 29 2018 at 12:31, on Zulip):

yes, seems like a great start

Yati Sagade (May 29 2018 at 12:34, on Zulip):

++

Yati Sagade (May 29 2018 at 12:39, on Zulip):

@Andrea Lattuada I've pushed a minor thing, if you are happy with what we have, I can squash our commits together so you can open a PR?

Andrea Lattuada (May 29 2018 at 12:54, on Zulip):

@Andrea Lattuada I've pushed a minor thing, if you are happy with what we have, I can squash our commits together so you can open a PR?

Sounds good!

Andrea Lattuada (May 29 2018 at 12:55, on Zulip):

I think they mean the way that we add the universal regions to region_live_at — that's only a temp hack, but it's true that it is presently modified

Correct. (I don’t think I can claim I know what many of those mean: sorry for being imprecise there)

Yati Sagade (May 29 2018 at 13:10, on Zulip):

@Andrea Lattuada pushed, I hope I have worded it right (still not 100% onboard with the terminology)

Andrea Lattuada (May 29 2018 at 13:18, on Zulip):

Cool! I’ll add you as co-author.

Andrea Lattuada (May 29 2018 at 13:21, on Zulip):

(Did you intend to squash all commits? Or was the intent to add a final commit? Either works, just checking what we should do there.)

Yati Sagade (May 29 2018 at 13:21, on Zulip):

I wante

Yati Sagade (May 29 2018 at 13:22, on Zulip):

*wanted to squash since otherwise the commits had quite terrible messages

Andrea Lattuada (May 29 2018 at 13:23, on Zulip):

Makes sense. It doesn’t look squashed on the branch, though, let me try and quickly fix that.

Yati Sagade (May 29 2018 at 13:27, on Zulip):

ouch indeed. I messed this one up. Just need to

Yati Sagade (May 29 2018 at 13:28, on Zulip):

*resquash and force push. sorry about that

Andrea Lattuada (May 29 2018 at 13:28, on Zulip):

Done, I think. Have a look and check it looks right. (you’ll need to reset)

Yati Sagade (May 29 2018 at 13:29, on Zulip):

That's perfect now

Yati Sagade (May 29 2018 at 13:29, on Zulip):

Thanks!

Andrea Lattuada (May 29 2018 at 13:32, on Zulip):

Thank you!

Yati Sagade (May 29 2018 at 14:05, on Zulip):

We broke the build, should've rebased onto master before, I'll force push an amended commit

Last update: Nov 21 2019 at 14:45UTC