Skip to content

Handle function declaration appearing after definition (fix #399) #411

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

Merged
merged 7 commits into from
Feb 8, 2021

Conversation

john-h-kastner
Copy link
Collaborator

Ensure that constrains generated due to a function's declaration are
applied to the variables generated at the definition even when
declaration appears after the definition. This is done by copying
the atoms from the definition constraint variable into the declaration
constraint variable using brainTransplant before any constraints are
added on the declaration.

Ensure that constrains generated due to a function's declaration are
applied to the variables generated at the definition even when
declaration appears after the definition.
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.

While trying to understand the code of this PR and confirm that it restores the symmetry between a declaration before and after the definition, I noticed a problem that pre-dated the PR: When we have a declaration and a definition of the same function in two different files with inconsistent parameter types, we should do the same consistency check as when we have two declarations, otherwise 3C may crash. An example:

mismatch1.c:

void foo(char *x);

mismatch2.c:

void foo(char **y) {
  // this is the body
}

Before this PR, 3c --output-postfix=scratch mismatch1.c mismatch2.c crashes, while 3c --output-postfix=scratch mismatch2.c mismatch1.c silently ignores the declaration (that's the problem this PR is intended to fix). With this PR, both cases crash. John, do you want to take this on here, or shall I file a separate issue?

I also notice that some of the existing code treats inconsistent declarations/definitions differently if one of them happens to have zero parameters (even when written explicitly as void foo(void) rather than void foo(), which is interpreted as having unspecified parameters). But I believe our decision was that if the input program is invalid, 3C just needs to not crash and its output is undefined.

It calls brainTransplant, which gets rid of all the atoms in the
destination constraint variable, losing any constraints on them in the
process.

Also fix two places where this could have potentially been an issue.
Previously it returned a bool, but the value was never used. This also
applies to two functions that are only ever called from inside
insertNewFVConstraint.
@john-h-kastner
Copy link
Collaborator Author

@mattmccutchen-cci I went ahead and made the functions return void. I think the real meaning of the boolean return value was lost at some point. It's supposed to indicate success, but that hasn't been maintained by various changes to the function (including this one).

Your comments about incompatible function declaration/definitions are best handled in a separate issue. I'm not sure how exactly we want to handle it, and I feel like we might run into more questions about what should happen during constraint variable merging that could quickly expand the scope of the PR.

@mattmccutchen-cci
Copy link
Member

mattmccutchen-cci commented Feb 3, 2021

Your comments about incompatible function declaration/definitions are best handled in a separate issue. I'm not sure how exactly we want to handle it, and I feel like we might run into more questions about what should happen during constraint variable merging that could quickly expand the scope of the PR.

I think the intended behavior for now is clear (just exit like with two incompatible declarations; it could get more complicated in the future if we wanted to recover gracefully and still process the definition), but I agree this could expand the scope of the PR, so I've filed #414.

All of my concerns have been addressed (thanks!), but I don't feel qualified to approve the PR as a whole.

Copy link
Member

@mwhicks1 mwhicks1 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 to me. I'm a little confused about why the "unifyIfTypedef" code has shown up, but I trust that it's OK. If it's consequential, I might have liked to see the impact of it in a new test.

@john-h-kastner john-h-kastner merged commit 785f962 into main Feb 8, 2021
@john-h-kastner john-h-kastner deleted the fix_399 branch March 9, 2021 15:41
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.

3 participants