Skip to content

Pragmas in declarations (volatile) [depends-on: #220] #215

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

Closed

Conversation

xbauch
Copy link
Contributor

@xbauch xbauch commented May 9, 2019

Handle volatile pragmas by updating the symbol in symbol table. Turned out the symbol wasn't there yet. Extended the Do_Defining_Identifier to insert the symbol.

The first commit is a squash of the prerequisite PR #220 -- no need to review it here.

Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

LGTM overall, just one question

Data := 0;
end if;

pragma Assert (Data=42);
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the behaviour of symex prior to the previous changes? Would this particular test have failed anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it would look exactly the same way. Only the symbol table would contain the volatile flag and gnat2goto would complain about not supporting pragma volatile.

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

Sorry to be a pain but please can we have some more test cases for the other issues that this seems to have resolved. I think a number of them have open tickets and it is not clear to me why the changes in this PR affect things like N_Attribute_Reference or N_Integer_Literal.

The handling of volatile seems fine for now.

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

Sorry; should have been request changes.

@xbauch xbauch force-pushed the feature/pragma-declaration-volatile branch from 645070d to 1ef713c Compare May 10, 2019 06:37
@xbauch xbauch changed the title Pragmas in declarations (volatile) [depends-on: #214] Pragmas in declarations (volatile) May 10, 2019
@xbauch
Copy link
Contributor Author

xbauch commented May 10, 2019

@martin-cs Those changes come from the first commit, where we store the identifier being defined in the symbol table. I created a stand-alone PR for that change: #220.

Petr Bauch added 4 commits May 10, 2019 09:16
via a separate method that looks-up the symbol and updates it.
to check that the symex does not choke (the symbols have the volatile flag set
in the symbol table).
@xbauch xbauch force-pushed the feature/pragma-declaration-volatile branch from 1ef713c to 789d901 Compare May 10, 2019 08:22
@xbauch xbauch changed the title Pragmas in declarations (volatile) Pragmas in declarations (volatile) [depends-on: #220] May 10, 2019
Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

The changes for this PR look good. I will have a look at #220 separately.

Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

LGTM

@chrisr-diffblue
Copy link
Contributor

I'll rebase this branch as a new PR and merge. I'll post a new comment here when it's done.

@chrisr-diffblue
Copy link
Contributor

The rebased version is in #246.

@chrisr-diffblue
Copy link
Contributor

Merged in #246.

@xbauch xbauch deleted the feature/pragma-declaration-volatile branch June 10, 2019 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants