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

Add forward progress update to NEWS.md #54089

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

LilithHafner
Copy link
Member

Closes #40009 which was left open because of the needs news tag.

NEWS.md Outdated Show resolved Hide resolved
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
NEWS.md Outdated Show resolved Hide resolved
@adienes
Copy link
Contributor

adienes commented Apr 15, 2024

are non-trivial inf loops UB?

@Keno
Copy link
Member

Keno commented Apr 15, 2024

Any loop that makes forward progress was never UB

NEWS.md Outdated
@@ -23,6 +23,8 @@ Language changes
expression within a given `:toplevel` expression to make use of macros
defined earlier in the same `:toplevel` expression. ([#53515])

- Trivial infinite loops (like `while true; end`) are no longer undefined behavior ([#52999])
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Trivial infinite loops (like `while true; end`) are no longer undefined behavior ([#52999])
- Trivial infinite loops (like `while true; end`) no longer cause invalid compiler optimizations ([#52999])

Can we get rid of the "Undefined Behavior" jargon? Julia — as I understand it — should not have UB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not jargon, it's precise. There's many constructs in julia that are UB - incorrect @inbounds, various poking at internal data structures, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, this is not a bugfix, it's a specification change for the language (even though we don't have the specification written out).

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my version of the unwritten spec this was a bug, though! :p

Seriously, though, unlike @inbounds, etc., there were no documented caveats to what while true; end meant. Invalid uses of @inbounds and internals muckery are clearly warned to be invalid and break things. That is something different than UB to me.

I maintain that UB is a compiler-writer's jargon — it has very specific understandings and expectations. It means not just that a result might be arbitrary/unspecified, but it means that the compiler can do wild and unexpected things because of that. It's why we freeze values from LLVM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it had been a bug, we would have just fixed it years ago and wouldn't needed #40009 to discuss the language semantics change. Would it be nice to have a more formal spec written down somewhere about what is or is not undefined behavior in the language? Sure! But in the meantime, you can ask the people who implemented the thing about what decisions were made about the language semantics. And in this case, the decision was originally to inherit LLVM's memory model with all that that implies and then we reconsidered that decision in #40009, which is what's being documented here. Once that decision was made #52999 indeed became a bugfix, but that bug fix doesn't really need NEWS. We fix way worse compiler bugs than that all the time. What needs NEWS is that we changed the semantics of the language.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retroactively update the unwritten Julia spec (unwritten specs are easy to update) so that this is in fact a bugfix and then it doesn't need a NEWS entry at all and just document the current "policy" somewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh, sure, but the same could be said about everything. The purpose of NEWS is to give people a heads up about changes that they may need to be aware of. Changing the forward progress guarantees means that people are likely to see perf regressions in cases where we previously were relying on these semantics, may need to update assume effects, may need to update external compiler passes, etc. That's the purpose of documenting changes in language semantics, so that when people upgrade to 1.12 and they see something weird here, they may remember reading about it in NEWS. That's also why I think it's important to use the precise and correct terminology here, which does include "undefined behavior" and "forward progress" so that people who need to care about it can use it to find the extensive related literature. The change of behavior for while true; end is the least interesting part of this (because people don't usually write code that way anyway). I don't really get what the problem is with just writing down the change in language semantics that we made.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote some devdocs on this: #54099. I still think the NEWS entry needs to be precise about what the actual change is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to include precise technical language if you provide a link to a webpage documenting what that language means (looking at the term "forward progress" here). The news should be accessible to folks who don't have the C spec nor Julia's unwritten spec memorized.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The news should be accessible to folks who don't have the C spec nor Julia's unwritten spec memorized.

Yes, this is exactly at the crux of why I'm suggesting these changes. The important thing to me is how Julia's behavior changes, because that's all we have to go on.

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

Successfully merging this pull request may close these issues.

Reconsider stance on forward progress guarantees
6 participants