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

Initialization of memory and variables #257

Merged
merged 39 commits into from
Jun 22, 2021
Merged

Initialization of memory and variables #257

merged 39 commits into from
Jun 22, 2021

Conversation

chandlerc
Copy link
Contributor

@chandlerc chandlerc commented Feb 4, 2021

This proposal outlines a suggested design for initialization in Carbon.
The early draft was developed in the document here:
https://docs.google.com/document/d/1UkDu1Wo5qsmedgt-gZyINdOV5Qc8nICpltZ_TZfTo40/edit#

Moving it here to begin the RFC process for this proposal.

This proposal outlines a suggested design for initialization in Carbon.
The early draft was developed in the document here:
https://docs.google.com/document/d/1UkDu1Wo5qsmedgt-gZyINdOV5Qc8nICpltZ_TZfTo40/edit#

Moving it here to begin the RFC process for this proposal.
@chandlerc chandlerc added proposal A proposal WIP labels Feb 4, 2021
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Feb 4, 2021
@chandlerc chandlerc added proposal rfc Proposal with request-for-comment sent out and removed WIP labels Feb 4, 2021
@chandlerc
Copy link
Contributor Author

Moving this to RFC. I think I still have one TODO left in the document, but I think the early draft has largely converged and its in a good state for folks to review!

Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

Just a few mechanical markdown conversion issues.

proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
chandlerc and others added 5 commits February 5, 2021 03:25
…247)

This isn't reflected in the above text, not sure how it managed to remain. There is still a note about filing issues for open questions at https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/consensus_decision_making.md#formal-decision-content, but I think it's much clearer when it's part of the core team's decision, rather than during proposal iteration.
Per chandlerc's comment:

First, this builds inside the Bazel tree rather than in the source
repository. This ensures we start in a clean directory each time the
repository rule is run again. We don't need to detect an existing build
with this, and it will only be rebuilt when the workspace file or the
repository rule implementation is changed. This can be forced by using:
```
bazel sync --configure
```

Second, we use an implicit dependency on the `WORKSPACE` file to locate
the workspace directory automatically, and the `HEAD` file from the
`llvm-project` submodule to trigger a rebuild if the submodule is
updated.

Third, teach the CMake script to try to use a system-installed `clang`
if installed and not overridden by the `CC` environment variable. This
is very different from the prior logic -- this is only used with the
CMake build, and so should work with any system C++ compiler that can
build Clang and LLVM.

Lastly, this tweaks the CMake options to tune this build given that we
now fully control it and it will only be used in this context. This
still results in a 1.5gb build for me. =/ But its as small as I can make
it really. It's a frustrating long list, but I couldn't find a more
brief way of representing this.

Co-authored-by: Chandler Carruth <[email protected]>
@chandlerc chandlerc requested a review from a team February 9, 2021 03:16
proposals/p0257.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mmdriley mmdriley left a comment

Choose a reason for hiding this comment

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

Commented throughout.

I read this proposal chiefly from a safety angle. Having gotten to the end, I'm worried that I would be just as likely to write code in Carbon that reads uninitialized values as if I were writing C or C++.

Does that disagree with others' understanding? Did I miss the forest for the trees?

proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
@chandlerc chandlerc requested a review from a team March 15, 2021 07:24
Copy link
Contributor Author

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Not done addressing comments, but wanted to publish at least a couple of important responses and then dig into a more real-time discussion with @mmdriley to make sure I understand the rest of what is needed here.

proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
@chandlerc
Copy link
Contributor Author

Thanks!

I think there are a couple of interesting threads left here -- at least the return of an unformed value one and the one around swap.

proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Show resolved Hide resolved
proposals/p0257.md Show resolved Hide resolved
@chandlerc
Copy link
Contributor Author

Thanks all for the great discussion. I've updated the document to be explicit and clear about all of the things we're precluding and added more rationale to the sections discussing those alternatives.

PTAL!

@chandlerc chandlerc requested a review from zygoloid June 18, 2021 04:53
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
proposals/p0257.md Outdated Show resolved Hide resolved
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Looks good, please proceed.

@chandlerc chandlerc merged commit 37dc922 into carbon-language:trunk Jun 22, 2021
@chandlerc chandlerc deleted the uninit branch June 22, 2021 04:34
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jun 22, 2021
@jonmeow jonmeow mentioned this pull request Jul 2, 2021
chandlerc added a commit that referenced this pull request Jun 28, 2022
This proposal outlines a suggested design for initialization in Carbon.

The early draft was developed in the document here:
https://docs.google.com/document/d/1UkDu1Wo5qsmedgt-gZyINdOV5Qc8nICpltZ_TZfTo40/edit#

Co-authored-by: josh11b <[email protected]>
Co-authored-by: Jon Meow <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: Geoff Romer <[email protected]>
zygoloid added a commit to zygoloid/carbon-lang that referenced this pull request May 31, 2023
…-or-nothing.

Disallow storing to subobjects of uninitialized objects.
jonmeow pushed a commit that referenced this pull request May 31, 2023
… checking (#2862)

Per #257, we should be treating unformedness as all-or-nothing, rather than being a per-field or per-array-element property. Previously we initialized an array with no explicit initializer as containing a sequence of uninitialized values, but that led to crashes when attempting to access those values, as the checks for reading an uninitialized value only expected values to be uninitialized at the top level.

Also, we had existing tests that attempt to store to an element of an uninitialized array. We now detect that and treat it as UB during evaluation, rather than crashing due to trying to perform field access into an uninitialized value.

Finally, many of these problems can be detected statically, but the resolve_unformed pass wasn't catching them because it missed a few expression and declaration forms. Support for those cases has been added too. This causes the pass to recurse more often, and in particular our existing recursion test started hitting a stack overflow after this, so resolve_unformed now uses `RunWithExtraStack`. In passing, remove the need to explicitly tell `RunWithExtraStack` the return type, and infer it as the return type of the callable instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants