Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Fix TCompLoop printing being stateful" #160

Closed
wants to merge 1 commit into from

Conversation

michael-schwarz
Copy link
Member

This reverts commit eded39e. (c.f. #134)

It causes non-termination of the printers (see goblint/analyzer#1290)

@sim642
Copy link
Member

sim642 commented Dec 13, 2023

Instead of just reverting, I think we should figure out why #134 should somehow cause non-termination because that's not clear at all. It's only removing things which weren't in the hashtable before and were just added.

By never removing them, the visited structs just accumulate and the extraneously accumulated structs somehow prune evaluation. But that pruned evaluation isn't then happening recursively from within the modified block, rather somewhere later in some sequential process. But that seems like something else odd is also going on.

@michael-schwarz
Copy link
Member Author

But that pruned evaluation isn't then happening recursively from within the modified block, rather somewhere later in some sequential process. But that seems like something else odd is also going on.

The sampling seems to point at the recursion happening entirely within pOnlyType.

@michael-schwarz
Copy link
Member Author

One idea: Maybe something is delayed by these doc things that are everywhere and then only evaluated later?

@sim642
Copy link
Member

sim642 commented Dec 13, 2023

One idea: Maybe something is delayed by these doc things that are everywhere and then only evaluated later?

That can't be because the Pretty.doc is just an ADT of strings and nothing delayable.

@michael-schwarz
Copy link
Member Author

Other possibility: Pretty uses Obj.magic, maybe breaking some assumptions of flambda. Though I think it is too early to suspect the compiler.

@michael-schwarz
Copy link
Member Author

Seems like it is working as intended (as @sim642 explains here goblint/analyzer#1290)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants