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

Implement _Atomic support #430

Closed
vit9696 opened this issue Aug 31, 2021 · 14 comments · Fixed by #431
Closed

Implement _Atomic support #430

vit9696 opened this issue Aug 31, 2021 · 14 comments · Fixed by #431

Comments

@vit9696
Copy link
Contributor

vit9696 commented Aug 31, 2021

As mentioned in #428 we need _Atomic keyword support for C11. This keyword is rather special, as it can be both a qualifier and a specifier. C standard provides two grammar definitions for this:

(6.7.2) type-specifer:
  void
  char
  short
  int
  long
  float
  double
  signed
  unsigned
  _Bool
  _Complex
  atomic-type-specifer
  struct-or-union-specifer
  enum-specifer
  typedef-name

(6.7.2.4) atomic-type-specifer:
  _Atomic ( type-name )
(6.7.3) type-qualifer:
  const
  restrict
  volatile
  _Atomic

Depending on whether the _Atomic keyword is a specifier or a qualifier it refers to an atomic type. The atomic type can contain another atomic type, i.e. define nested atomic types, and it can also refer to specific parts of the type. As mentioned in the original PR all the following examples are valid:

_Atomic(int *) a;
_Atomic(int) *b;
_Atomic int *c;

All these samples need to generate different code after being parsed through pycparser and converted back to C, so the implementation needs to treat _Atomic qualifier and specifier separately. My suggestion is to follow the grammar of the C standard and use a normal qualifier for _Atomic qualifier, and a dedicated class like with enum.

@eliben
Copy link
Owner

eliben commented Aug 31, 2021

In your example, aren't b and c equivalent semantically?
And isn't a the same as int * _Atomic c?

I'm thinking of two aspects here:

  1. Parsing to get to the AST
  2. AST semantics

W.r.t. the former, you'll need code to parse the two different forms of _Atomic anyway.
W.r.t. the latter, the AST's goal is to represent the semantic meaning of the program, not its syntax. It's somewhat canonical. There are many decisions in pycparser where different syntaxes parse into the same AST. For example, the two following C samples generate the same AST:

int x, y;
int x;
int y;

While they have different syntax, they have the same semantics.

So my real question is: do the _Atomic specifier and qualifier represent different semantics, or just different syntax for the same semantics. If the former, then adding new AST nodes makes sense. If the latter, it doesn't.

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 1, 2021

Alright, I think I get what you mean. In this example the following holds:

_Atomic(int *) a;
_Atomic(int) *b;
_Atomic int *c;
int * _Atomic d;
int _Atomic *e;

a = d;
b = c = e;

So they are:

  1. Different to parse.
  2. Semantically equivalent in a program.

Part 1 and 2 match what you said, so I believe I see now why you do not want a keyword. Is the suggestion for _Atomic to build AST in a way only one form can be then generated? Presumably the specifier form.

I personally do not like this as I believe the understanding of CST and AST is a bit extreme in your example. I also do not think it is any good to generate different code in this particular case as it may help with some styling warnings. However, I am not going to fight with this as long as you provide some clear explanations on the route to take.

  • What was your suggested approach to do it taking this new knowledge in consideration?
  • How important is it not to break the existing code and examples?
  • Can I handle _Alignas and _Alignof together with _Atomic?

@eliben
Copy link
Owner

eliben commented Sep 1, 2021

"why you do not want a keyword."

Just to clarify; I do not want an AST node. A keyword for _Atomic is required for the parsing.

To answer your questions:

  1. The qualifier form doesn't present challenges. The _Atomic(type) a form would do something like - get the type parsed from type, attach an _Atomic qualifier on it. You already had the parsing code in your PR, no? All I'm suggesting is that instead of creating a new AST node from it, you attach a qualifier to the underlying type.

  2. Not sure what you mean by this. What existing code / examples would be broken?

  3. _Alignof is like sizeof, no? As for _Alignas, I haven't considered it in depth yet. Is it similar to _Atomic?

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 1, 2021

  1. Well, qualifier form does not allow one to express nested atomics, does it? E.g. _Atomic(_Atomic(int) *)). Do not we need to use a specifier form everywhere to implement your approach? Could you write AST for this example and the examples above?
  2. This is about Alignas, I had to add a new specifier class to support it (https://github.com/eliben/pycparser/pull/428/files#diff-3354e31b7e9559489b86707d0040ec8ffa678db98249c8f55f6a751381f927f1R358). If we go your route with _Atomic, I assume something similar will be needed.
  3. I guess it is somewhat similar to _Atomic(x) variant.

@eliben
Copy link
Owner

eliben commented Sep 1, 2021

How is this nesting in _Atomic different from const?

You can write const int * const p;, so can't you write _Atomic int * _Atomic p?

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 1, 2021

Hrrrm, I think I see it finally. I believe I had a fundamental misunderstanding of how C grammar works in regards to Atomic. I did not make a parallel to const here, and thought _Atomic(x) specifier syntax was suggested to lift some semantical restrictions.

I read http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1485.pdf and realised that actually I was wrong, and the specifier is here solely for convenience reasons. In my defence I can add that it is quite strange to have two syntaxes with the same expressibility, but oh well. I think I can work with this.

eliben added a commit that referenced this issue Sep 13, 2021
This adds initial implementation for the _Atomic keyword in C11, only focusing
on the use as qualifier (spec section 6.7.3)

Based on #431 by vit9696. Updates #430
eliben added a commit that referenced this issue Sep 13, 2021
@eliben
Copy link
Owner

eliben commented Sep 13, 2021

I went ahead a committed a couple of changes to implement _Atomic. The relevant AST logic in the parser is very convoluted (due to the bottom-up nature of LR parsing, thanks Yacc....) so it was easier to dig this through myself than to explain what I mean on the other PR. It's a bit hacky, but it seems to work for basic tests.

Note that I didn't port over the C generator tests from #431 and I didn't port over your addition of fake headers + end-to-end test. Feel free to update #431 for that, or to create a separate PR. Specifically, the C generator may not work correctly for the new atomic specifiers.

Also note that paragraph 3 of section 6.7.2.4 in the standard forbids nesting of the atomic specifier, so something like _Atomic(_Atomic(int) *) p is invalid. Also, qualified types can't go into the _Atomic specifier.

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 13, 2021

Hi, thanks for your work!

I will check it more precisely in the coming days, but for the time being there is an understanding flaw.

Also note that paragraph 3 of section 6.7.2.4 in the standard forbids nesting of the atomic specifier, so something like _Atomic(_Atomic(int) *) p is invalid. Also, qualified types can't go into the _Atomic specifier.

I am afraid you might misinterpreted it.

The type name in an atomic type specifer shall not refer to an array type, a function type, an atomic type, or a qualifed type.

Pointer to an atomic is not an atomic type itself, it is non-atomic. _Atomic(_Atomic(int)) p is not valid, but _Atomic(_Atomic(int) *) p is thus perfectly valid.

@eliben
Copy link
Owner

eliben commented Sep 14, 2021

Thanks, that makes sense. I've committed another fix. When you look at it, make sure to look at the latest code in master. Feel free to add more tests in your followup PR.

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 15, 2021

Hmmmm, I tried, but the implementation is still incorrect, as typedef _Atomic(_Bool) atomic_bool; does not parse. This comes from e.g. LLVM <stdatomic.h>. I believe that is what my following code handled:

    # This is a bit ugly, but we need to process atomic specifier before qualifiers,
    # since _Atomic(int) has a conflict with _Atomic int.
    def p_declaration_specifiers_no_type_4(self, p):
        """ declaration_specifiers_no_type  : atomic_specifier declaration_specifiers_no_type_opt
        """
        p[0] = self._add_declaration_specifier(p[2], p[1], 'type')

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 15, 2021

With this diff it does work, but I am not positive whether this is what you intended to do:

% git diff
diff --git a/pycparser/c_parser.py b/pycparser/c_parser.py
index 7d087f0..4b16457 100644
--- a/pycparser/c_parser.py
+++ b/pycparser/c_parser.py
@@ -779,6 +779,13 @@ class CParser(PLYParser):
         """
         p[0] = self._add_declaration_specifier(p[2], p[1], 'function')
 
+    # This is a bit ugly, but we need to process atomic specifier before qualifiers,
+    # since _Atomic(int) has a conflict with _Atomic int.
+    def p_declaration_specifiers_no_type_4(self, p):
+        """ declaration_specifiers_no_type  : atomic_specifier declaration_specifiers_no_type_opt
+        """
+        p[0] = self._add_declaration_specifier(p[2], p[1], 'type')
+
     def p_declaration_specifiers_1(self, p):
         """ declaration_specifiers  : declaration_specifiers type_qualifier
         """
@@ -857,6 +864,15 @@ class CParser(PLYParser):
         typ.quals.append('_Atomic')
         p[0] = typ
 
+    # See section 6.7.2.4 of the C11 standard.
+    def p_type_specifier_3(self, p):
+        """ atomic_specifier  : _ATOMIC LPAREN type_name RPAREN
+        """
+        typ = p[3]
+        typ.quals.append('_Atomic')
+        p[0] = typ
+
+
     def p_type_qualifier(self, p):
         """ type_qualifier  : CONST
                             | RESTRICT

@eliben
Copy link
Owner

eliben commented Sep 16, 2021

Right, this is a problem for all storage class specifiers like auto and extern as well, for the reason you stated. The declaration_specifiers_no_type rule exists due to earlier complications in the grammar, and yacc grammars won't backtrack.

I'm OK with a PR to add your workaround; just please make sure to add several tests with different storage class specifiers.

Also if you introduce atomic_specifier once there shouldn't be duplication (it can be used in both rules).

@vit9696
Copy link
Contributor Author

vit9696 commented Sep 17, 2021

I'm OK with a PR to add your workaround; just please make sure to add several tests with different storage class specifiers.

Alright, added, though I believe it failed again in an interesting manner for generator tests. See the updated PR.

Also if you introduce atomic_specifier once there shouldn't be duplication (it can be used in both rules).

Yeah, did that. Actually had that earlier, just crafted something simple for a PoC.

@eliben
Copy link
Owner

eliben commented Sep 17, 2021

The qualifier duplication on Decl could be an issue in general, something that should be discussed separately.

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 a pull request may close this issue.

2 participants