Skip to content

Changes to add itypes on undefined functions #691

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Sep 8, 2021
Merged

Changes to add itypes on undefined functions #691

merged 17 commits into from
Sep 8, 2021

Conversation

john-h-kastner
Copy link
Collaborator

@john-h-kastner john-h-kastner commented Aug 25, 2021 •
edited
Loading

With this change, 3C will insert itypes on functions without definitions. For example:

void foo(int *);

becomes

void foo(int * : itype(_Ptr<int>));

This should be merged into main after #690, #689, and #688.

kyleheadley
kyleheadley approved these changes Aug 26, 2021
View reviewed changes
Copy link
Member

@kyleheadley kyleheadley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good, though I had couple of comments.
Congratulations on getting more checked types in the tests.

john-h-kastner added a commit that referenced this pull request Aug 26, 2021
@john-h-kastner
Ensure bounds are not inserted into unwritable files (#689)
d7b9b2d 
Adds checks to ensure that 3C will not attempt to rewrite in unwritable files
to insert array bounds. The test file in this PR contains an example using a
function body in an unwritable file which fails on the current main. After
changes made to support rewriting in functions declarations without
definitions (#691), this will be able to
happen even when the functions are not defined, and so it will becomes a more
prevalent issue.
@john-h-kastner john-h-kastner changed the base branch from rewrite_extern.base to main August 27, 2021 19:27
@john-h-kastner john-h-kastner changed the base branch from main to options_cleanup August 30, 2021 18:50
@john-h-kastner
Copy link
Collaborator Author

The new behavior is now behind a flag. This now also depends on #692

@john-h-kastner john-h-kastner changed the base branch from options_cleanup to main August 30, 2021 19:55
@john-h-kastner
Undo obsolete change that was needed before other implementation changes
a205b0f
@john-h-kastner
Expand test coverage
0ec32e0
@john-h-kastner
Generate itype string from AST when extracting from source fails
080f543 
Fixes an assertion that would fail when attempting to extract the string
representation of an itype expression from the original source code if
the itype expression was inside a macro. The string representation for
itypes in macros is now re-generated from the AST.
@john-h-kastner
Comment edits
cc9024a
mattmccutchen-cci
mattmccutchen-cci reviewed Aug 31, 2021
View reviewed changes
Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

The flag definition itself looks OK modulo my comment, but there have been so many merges and force-pushes since Kyle's approval that I can't claim to have reviewed the other changes you've made since then. Are we going to just trust that those changes are OK? Or if we want, I can try to construct the right diff to review.

@john-h-kastner
Copy link
Collaborator Author

john-h-kastner commented Aug 31, 2021 •
edited
Loading

Or if we want, I can try to construct the right diff to review.

The relevant changes should just be d589534...0ec32e0

The force-pushing can safely be ignored.

john-h-kastner and others added 2 commits August 31, 2021 15:14
@john-h-kastner @mattmccutchen-cci
Update clang/tools/3c/3CStandalone.cpp
fde7bda 
Co-authored-by: Matt McCutchen (Correct Computation) <matt@correctcomputation.com>
@john-h-kastner
Update test
524eb39
@mattmccutchen-cci
Copy link
Member

The relevant changes should just be d589534...0ec32e0

Thanks. @kyleheadley I think it makes the most sense if you review these changes since you reviewed the previous version of the same code.

@kyleheadley
Copy link
Member

kyleheadley commented Sep 1, 2021 •
edited
Loading

The relevant changes should just be d589534...0ec32e0

In this block I see:

ConstraintVariables ~2260:
-  bool SrcChecked = ExternalConstraint->isOriginallyChecked() ||
-                   ExternalConstraint->srcHasItype();
+  bool HasItype = ExternalConstraint->srcHasItype();

But it doesn't appear in the collective change. It's the only code from the block that I have any issue with. Was it perhaps reversed because of how github deals with diffs? Was this a real change and if so what is it for?

@john-h-kastner
Copy link
Collaborator Author

Was it perhaps reversed because of how github deals with diffs? Was this a real change and if so what is it for?

This is undoing a change that was present in the original version of the PR

mattmccutchen-cci
mattmccutchen-cci reviewed Sep 7, 2021
View reviewed changes
Copy link
Member

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

One thing I noticed. I'm still hoping Kyle will review all the changes since his previous approval.

Also, please merge main into this branch per our discussion. We expect that to make no difference, but it's good to rule out any surprises during the PR merge.

clang/lib/3C/ProgramInfo.cpp Outdated
Comment on lines 457 to 458
// If we've seen this symbol, but never seen a body for it, constrain
// everything about it.
Copy link
Member

Choose a reason for hiding this comment

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

It seems this comment is no longer quite accurate when InferTypesForUndefs is on.

@kyleheadley
Copy link
Member

One thing I noticed. I'm still hoping Kyle will review all the changes since his previous approval.

Did I not do that in my previous comment?

If there's some confusion about what needs to be reviewed, please send me a link, like was done above, or let me know that I'll need to look over the whole thing again. Otherwise, I'll stick with trusting that our team wants to have their code reviewed and has made their best effort to provide it. Since, as far I as recall, "submitter presses merge" and takes responsibility for their own code.

@mattmccutchen-cci
Copy link
Member

One thing I noticed. I'm still hoping Kyle will review all the changes since his previous approval.

Did I not do that in my previous comment?

If there's some confusion about what needs to be reviewed, please send me a link, like was done above, or let me know that I'll need to look over the whole thing again. [...]

My concern was that that discussion had not reached a conclusion (as could be expressed by a GitHub re-approval), not that there was confusion about what needed review. Maybe I should have said "approve" rather than "review". 🙂

kyleheadley
kyleheadley approved these changes Sep 7, 2021
View reviewed changes
Copy link
Member

@kyleheadley kyleheadley left a comment

Choose a reason for hiding this comment

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

re-approve (code so far). Carry on.

@john-h-kastner john-h-kastner merged commit 584c6ba into main Sep 8, 2021
This was referenced Sep 9, 2021
Closed
Open
@kyleheadley kyleheadley mentioned this pull request Sep 14, 2021
Draft
@Machiry Machiry deleted the rewrite_extern branch January 22, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmccutchen-cci mattmccutchen-cci mattmccutchen-cci left review comments

@kyleheadley kyleheadley kyleheadley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@john-h-kastner @mattmccutchen-cci @kyleheadley