Re: [PATCH] __VA_OPT__ fixes (PR preprocessor/83063, PR preprocessor/83708)

2018-02-15 Thread Jason Merrill 
On Thu, Feb 15, 2018 at 4:27 AM, Jakub Jelinek  wrote:
> On Thu, Feb 15, 2018 at 01:12:08AM -0500, Jason Merrill wrote:
>> > This is just a partial fix, one thing this patch doesn't change is that
>> > the standard says that __VA_OPT__ ( contents ) should be treated as
>> > parameter, which means that #__VA_OPT__ ( contents ) should stringify it,
>> > which we right now reject.  My preprocessor knowledge is too limited to
>> > handle this right myself, including all the corner cases, e.g. one can have
>> > #define f(x, ...) #__VA_OPT__(#x x ## x) etc..  I presume
>> > m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE);
>> > could be changed into:
>> > m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE | STRINGIFY_ARG);
>> > and when handling the PADDING result from update, we could just emit a
>> > "" token, but for INCLUDE_FIRST with this we'd need something complex,
>> > probably a new routine similar to stringify_arg to some extent.
>>
>> Yes, I think long term we really need to treat __VA_OPT__ more like an
>> argument.
>>
>> The first patch below makes your testcases work in what seems to me a
>> simpler way: pad when we see __VA_OPT__ if we aren't pasting to the left,
>> and fix up the end of the body if we're pasting to the right.
>>
>> The second further patch below makes the rest of the clang testcase work the
>> way it does in clang, apart from stringification.  But it feels more
>> kludgey.
>>
>> Thoughts?
>
> Both patches LGTM, thanks for looking at this.  If you apply the second patch,
> you might want to apply also following incremental patch with some additional
> tests from my (failed) attempt to extend the patch further (this passes with
> your second patch).

Great, thanks.  I kept poking at it this morning; instead of checking
the last emitted token for PASTE_LEFT, this version checks whether
it's at the beginning of __VA_OPT__, which seems safer.

Tested x86_64-pc-linux-gnu, applying to trunk.
commit 4a0e6c48c397f4cd6bb654c770f4d8d97de015cc
Author: Jason Merrill 
Date:   Wed Feb 14 13:59:22 2018 -0500

PR preprocessor/83063 - __VA_OPT__ and ##

PR preprocessor/83708
* macro.c (vaopt_state): Reorder m_last_was_paste before m_state.
(vaopt_state::vaopt_state): Adjust.
(vaopt_state::update_flags): Add BEGIN and END.
(vaopt_state::update): Return them.
(copy_paste_flag): Factor out of replace_args.
(last_token_is): New.
(replace_args): Handle BEGIN and END.  Avoid padding there.
(tokens_buff_last_token_ptr): Return NULL if no tokens.

diff --git a/libcpp/macro.c b/libcpp/macro.c
index f994ac584cc..776af7bd00e 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -105,8 +105,8 @@ class vaopt_state {
 : m_pfile (pfile),
 m_allowed (any_args),
 m_variadic (is_variadic),
-m_state (0),
 m_last_was_paste (false),
+m_state (0),
 m_paste_location (0),
 m_location (0)
   {
@@ -116,7 +116,9 @@ class vaopt_state {
   {
 ERROR,
 DROP,
-INCLUDE
+INCLUDE,
+BEGIN,
+END
   };
 
   /* Given a token, update the state of this tracker and return a
@@ -139,7 +141,7 @@ class vaopt_state {
  }
++m_state;
m_location = token->src_loc;
-   return DROP;
+   return BEGIN;
   }
 else if (m_state == 1)
   {
@@ -191,7 +193,7 @@ class vaopt_state {
return ERROR;
  }
 
-   return DROP;
+   return END;
  }
  }
return m_allowed ? INCLUDE : DROP;
@@ -220,6 +222,9 @@ class vaopt_state {
   bool m_allowed;
   /* True if the macro is variadic.  */
   bool m_variadic;
+  /* If true, the previous token was ##.  This is used to detect when
+ a paste occurs at the end of the sequence.  */
+  bool m_last_was_paste;
 
   /* The state variable:
  0 means not parsing
@@ -228,9 +233,6 @@ class vaopt_state {
  >= 3 means looking for ")", the number encodes the paren depth.  */
   int m_state;
 
-  /* If true, the previous token was ##.  This is used to detect when
- a paste occurs at the end of the sequence.  */
-  bool m_last_was_paste;
   /* The location of the paste token.  */
   source_location m_paste_location;
 
@@ -1701,6 +1703,30 @@ expanded_token_index (cpp_reader *pfile, cpp_macro 
*macro,
   return cur_replacement_token - macro->exp.tokens;
 }
 
+/* Copy whether PASTE_LEFT is set from SRC to *PASTE_FLAG.  */
+
+static void
+copy_paste_flag (cpp_reader *pfile, const cpp_token **paste_flag,
+const cpp_token *src)
+{
+  cpp_token *token = _cpp_temp_token (pfile);
+  token->type = (*paste_flag)->type;
+  token->val = (*paste_flag)->val;
+  if (src->flags & PASTE_LEFT)
+token->flags = (*paste_flag)->flags | PASTE_LEFT;
+  else
+token->flags = (*paste_flag)->flags & ~PASTE_LEFT;
+  *paste_flag = token;
+}
+
+/* True IFF the last token emitted

Re: [PATCH] __VA_OPT__ fixes (PR preprocessor/83063, PR preprocessor/83708)

2018-02-15 Thread Jakub Jelinek 
On Thu, Feb 15, 2018 at 01:12:08AM -0500, Jason Merrill wrote:
> > This is just a partial fix, one thing this patch doesn't change is that
> > the standard says that __VA_OPT__ ( contents ) should be treated as
> > parameter, which means that #__VA_OPT__ ( contents ) should stringify it,
> > which we right now reject.  My preprocessor knowledge is too limited to
> > handle this right myself, including all the corner cases, e.g. one can have
> > #define f(x, ...) #__VA_OPT__(#x x ## x) etc..  I presume
> > m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE);
> > could be changed into:
> > m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE | STRINGIFY_ARG);
> > and when handling the PADDING result from update, we could just emit a
> > "" token, but for INCLUDE_FIRST with this we'd need something complex,
> > probably a new routine similar to stringify_arg to some extent.
> 
> Yes, I think long term we really need to treat __VA_OPT__ more like an
> argument.
> 
> The first patch below makes your testcases work in what seems to me a
> simpler way: pad when we see __VA_OPT__ if we aren't pasting to the left,
> and fix up the end of the body if we're pasting to the right.
> 
> The second further patch below makes the rest of the clang testcase work the
> way it does in clang, apart from stringification.  But it feels more
> kludgey.
> 
> Thoughts?

Both patches LGTM, thanks for looking at this.  If you apply the second patch,
you might want to apply also following incremental patch with some additional
tests from my (failed) attempt to extend the patch further (this passes with
your second patch).

--- gcc/testsuite/c-c++-common/cpp/va-opt-3.c   2018-01-09 17:20:14.985142201 
+0100
+++ gcc/testsuite/c-c++-common/cpp/va-opt-3.c   2018-01-09 17:54:17.564372639 
+0100
@@ -19,6 +19,15 @@
 #define f13(...) __VA_OPT__(a)__VA_OPT__(b)c
 #define f14(a, b, c, ...) __VA_OPT__(a)__VA_OPT__(b)c
 #define f15(a, b, c, ...) __VA_OPT__(a b)__VA_OPT__(b c)a/**/__VA_OPT__(c a)a
+#define m1 (
+#define f16() f17 m1 )
+#define f17() f18 m1 )
+#define f18() m2 m1 )
+#define m3f17() g
+#define f19(x, ...) m3 ## __VA_OPT__(x x f16() #x)
+#define f20(x, ...) __VA_OPT__(x x)##m4()
+#define f21() f17
+#define f17m4() h
 t1 f1 (1, 2, 3);
 /* { dg-final { scan-file va-opt-3.i "t1 bc;" } } */
 t2 f1 ();
@@ -69,0 +79,4 @@
+t25 f19 (f16 (), 1);
+/* { dg-final { scan-file va-opt-3.i "t25 g f18 \\( \\) f17 \\( \\) \"f16 
\\(\\)\";" } } */
+t26 f20 (f21 (), 2);
+/* { dg-final { scan-file va-opt-3.i "t26 f17 h;" } } */


Jakub

Re: [PATCH] __VA_OPT__ fixes (PR preprocessor/83063, PR preprocessor/83708)

2018-02-14 Thread Jason Merrill 

On 01/10/2018 07:04 AM, Jakub Jelinek wrote:

The following patch attempts to fix various issues, including some ICEs,
by introducing 3 new states, two of them are alternatives to INCLUDE used
for the very first token after __VA_OPT__( , where we want to take into
account also flags from the __VA_OPT__ token, and before the closing )
token where we want to use flags from the closing ) token.  Plus
PADDING which is used for the case where there are no varargs and __VA_OPT__
is supposed to fold into a placemarker, or for the case of __VA_OPT__(),
which is similar to that, in both cases we need to take into account in
those cases both flags from __VA_OPT__ and from the closing ).


I had an idea for a way to simplify this, which I've attached below.


This is just a partial fix, one thing this patch doesn't change is that
the standard says that __VA_OPT__ ( contents ) should be treated as
parameter, which means that #__VA_OPT__ ( contents ) should stringify it,
which we right now reject.  My preprocessor knowledge is too limited to
handle this right myself, including all the corner cases, e.g. one can have
#define f(x, ...) #__VA_OPT__(#x x ## x) etc..  I presume
m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE);
could be changed into:
m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE | STRINGIFY_ARG);
and when handling the PADDING result from update, we could just emit a
"" token, but for INCLUDE_FIRST with this we'd need something complex,
probably a new routine similar to stringify_arg to some extent.


Yes, I think long term we really need to treat __VA_OPT__ more like an 
argument.


The first patch below makes your testcases work in what seems to me a 
simpler way: pad when we see __VA_OPT__ if we aren't pasting to the 
left, and fix up the end of the body if we're pasting to the right.


The second further patch below makes the rest of the clang testcase work 
the way it does in clang, apart from stringification.  But it feels more 
kludgey.


Thoughts?

Jason
commit f07a7a181489ad2c6287c239d6b034a3933a56ce
Author: Jason Merrill 
Date:   Wed Feb 14 13:59:22 2018 -0500

PR preprocessor/83063

PR preprocessor/83708
* macro.c (vaopt_state): Reorder m_last_was_paste before m_state.
(vaopt_state::vaopt_state): Adjust.
(vaopt_state::update_flags): Add BEGIN and END.
(vaopt_state::update): Return them.
(retcon_paste_flag, padding_ok_after_last_token): New.
(replace_args): Handle BEGIN and END.
(tokens_buff_last_token_ptr): Return NULL if no tokens.

diff --git a/gcc/testsuite/c-c++-common/cpp/va-opt-2.c b/gcc/testsuite/c-c++-common/cpp/va-opt-2.c
new file mode 100644
index 000..cff2d6cbe5d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/va-opt-2.c
@@ -0,0 +1,41 @@
+/* PR preprocessor/83063 */
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99" { target c } } */
+/* { dg-options "-std=c++2a" { target c++ } } */
+
+#define f1(...) int b##__VA_OPT__(c)
+#define f2(...) int __VA_OPT__(c)##d
+#define f3(...) int e##__VA_OPT__()
+#define f4(...) int __VA_OPT__()##f
+#define f5(...) int g##__VA_OPT__(h)##i
+#define f6(...) int j##__VA_OPT__()##k
+#define f7(...) int l##__VA_OPT__()
+#define f8(...) int __VA_OPT__()##m
+#define f9(...) int n##__VA_OPT__()##o
+#define f10(x, ...) int x##__VA_OPT__(x)
+#define f11(x, ...) int __VA_OPT__(x)##x
+#define f12(x, ...) int x##__VA_OPT__(x)##x
+f1 (1, 2, 3);
+f1 ();
+f2 (1, 2);
+f2 ();
+f3 (1);
+f4 (2);
+f5 (6, 7);
+f5 ();
+f6 (8);
+f7 ();
+f8 ();
+f9 ();
+f10 (p, 5, 6);
+f10 (p);
+f11 (q, 7);
+f11 (q);
+f12 (r, 1, 2, 3, 4, 5);
+f12 (r);
+
+int
+main ()
+{
+  return bc + b + cd + d + e + f + ghi + gi + jk + l + m + no + pp + p + qq + q + rrr + rr;
+}
diff --git a/gcc/testsuite/c-c++-common/cpp/va-opt-3.c b/gcc/testsuite/c-c++-common/cpp/va-opt-3.c
new file mode 100644
index 000..2e639891070
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cpp/va-opt-3.c
@@ -0,0 +1,69 @@
+/* PR preprocessor/83063 */
+/* PR preprocessor/83708 */
+/* { dg-do preprocess } */
+/* { dg-options "-std=gnu99" { target c } } */
+/* { dg-options "-std=c++2a" { target c++ } } */
+
+#define f1(...) b##__VA_OPT__(c)
+#define f2(...) __VA_OPT__(c)##d
+#define f3(...) e##__VA_OPT__()
+#define f4(...) __VA_OPT__()##f
+#define f5(...) g##__VA_OPT__(h)##i
+#define f6(...) j##__VA_OPT__()##k
+#define f7(...) l##__VA_OPT__()
+#define f8(...) __VA_OPT__()##m
+#define f9(...) n##__VA_OPT__()##o
+#define f10(x, ...) x##__VA_OPT__(x)
+#define f11(x, ...) __VA_OPT__(x)##x
+#define f12(x, ...) x##__VA_OPT__(x)##x
+#define f13(...) __VA_OPT__(a)__VA_OPT__(b)c
+#define f14(a, b, c, ...) __VA_OPT__(a)__VA_OPT__(b)c
+#define f15(a, b, c, ...) __VA_OPT__(a b)__VA_OPT__(b c)a/**/__VA_OPT__(c a)a
+t1 f1 (1, 2, 3);
+/* { dg-final { scan-file va-opt-3.i "t1 bc;" } } */
+t2 f1 ();
+/* { dg-final { scan-file va-opt-3.i "t2 b;" } } */
+t3 f2 (1, 2);
+/* { dg-final {

Re: [PATCH] __VA_OPT__ fixes (PR preprocessor/83063, PR preprocessor/83708)

2018-02-14 Thread Jason Merrill 

On 01/10/2018 07:04 AM, Jakub Jelinek wrote:

I've also cross-checked the libcpp implementation with this patch against
trunk clang which apparently also implements __VA_OPT__ now, on the
testcases included here the output is the same and on their
macro_vaopt_expand.cpp testcase, if I remove all tests that test
#__VA_OPT__ ( contents ) handling which we just reject now, there are still
some differences:
$ /usr/src/llvm/obj8/bin/clang++  -E /tmp/macro_vaopt_expand.cpp -std=c++2a > 
/tmp/1
$ ~/src/gcc/obj20/gcc/cc1plus -quiet -E /tmp/macro_vaopt_expand.cpp -std=c++2a 
> /tmp/2
diff -up /tmp/1 /tmp/2
-4: f(0 )
+4: f(0)
-6: f(0, a )
-7: f(0, a )
+6: f(0, a)
+7: f(0, a)
-9: TONG C ( ) B ( ) "A()"
+9: HT_A() C ( ) B ( ) "A()"
-16: S foo ;
+16: S foo;
-26: B1
-26_1: B1
+26: B 1
+26_1: B 1
-27: B11
-27_1: BexpandedA0 11
-28: B11
+27: B 11
+27_1: BA0 11
+28: B 11

Perhaps some of the whitespace changes aren't significant, but 9:, and
2[678]{,_1}: are significantly different.
9: is
#define LPAREN (
#define A() B LPAREN )
#define B() C LPAREN )
#define HT_B() TONG
#define F(x, ...) HT_ ## __VA_OPT__(x x A()  #x)
9: F(A(),1)

Thoughts on what is right and why?


clang is.  First we substitute into the body of the __VA_OPT__, so

x x A() #x
B ( ) B ( ) A() "A()"

Then we paste, so

HT_B ( ) B ( ) A() "A()"

then rescan, so

TONG C ( ) B ( ) "A()"


Similarly for expansion on the last token from __VA_OPT__ when followed
by ##, like:
#define m1 (
#define f16() f17 m1 )
#define f17() f18 m1 )
#define f18() m2 m1 )
#define m3f17() g
#define f19(x, ...) m3 ## __VA_OPT__(x x f16() #x)
#define f20(x, ...) __VA_OPT__(x x)##m4()
#define f21() f17
#define f17m4() h
t25 f19 (f16 (), 1);
t26 f20 (f21 (), 2);

E.g. 26: is:
#define F(a,...)  __VA_OPT__(B a ## a) ## 1
26: F(,1)
I really wonder why clang emits B1 in that case, there
is no ## in between B and a, so those are 2 separate tokens
separated by whitespace, even when a ## a is a placemarker.
Does that mean __VA_OPT__ should throw away all the placemarkers
and return the last non-placemarker token for the ## handling?


This is unclear to me.  The standard says that the replacement for the 
__VA_OPT__ is the result of expansion before rescanning and further 
replacement, but it's unclear to me whether the discarding of 
placemarkers should happen or not, since that is mentioned in the 
context of rescanning.  Representing a placemarker as <> below, we first 
expand the __VA_OPT__ body to


B a ## a
B <> ## <>
B <>

then do we have

B <> ## 1
or
B ## 1
?

Looking at the patch now.

Jason

Ping^2: Re: [PATCH] __VA_OPT__ fixes (PR preprocessor/83063, PR preprocessor/83708)

2018-01-26 Thread Jakub Jelinek 
Ping^2
http://gcc.gnu.org/ml/gcc-patches/2018-01/msg00727.html

On Wed, Jan 17, 2018 at 05:47:12PM +0100, Jakub Jelinek wrote:
> I'd like to ping this patch.
> As I wrote, it isn't a full solution for all the __VA_OPT__ issues,
> but it isn't even clear to me how exactly it should behave, but fixes
> some ICEs and a couple of most important issues and shouldn't make things
> worse, at least on the gcc and clang __VA_OPT__ testcases.

Jakub

Ping: Re: [PATCH] __VA_OPT__ fixes (PR preprocessor/83063, PR preprocessor/83708)

2018-01-17 Thread Jakub Jelinek 
Hi!

I'd like to ping this patch.
As I wrote, it isn't a full solution for all the __VA_OPT__ issues,
but it isn't even clear to me how exactly it should behave, but fixes
some ICEs and a couple of most important issues and shouldn't make things
worse, at least on the gcc and clang __VA_OPT__ testcases.

On Wed, Jan 10, 2018 at 01:04:07PM +0100, Jakub Jelinek wrote:
> In libcpp, we have quite a lot of state on the token flags, some
> related to the stuff that comes before the token (e.g.
> PREV_FALLTHROUGH, PREV_WHITE and STRINGIFY_ARG), others related to the
> stuff that comes after the token (e.g. PASTE_LEFT, SP_DIGRAPH, SP_PREV_WHITE).
> Unfortunately, with the current __VA_OPT__ handling that information is
> lost, because it is on the __VA_OPT__ or closing ) tokens that we are always
> DROPing.
> 
> The following patch attempts to fix various issues, including some ICEs,
> by introducing 3 new states, two of them are alternatives to INCLUDE used
> for the very first token after __VA_OPT__( , where we want to take into
> account also flags from the __VA_OPT__ token, and before the closing )
> token where we want to use flags from the closing ) token.  Plus
> PADDING which is used for the case where there are no varargs and __VA_OPT__
> is supposed to fold into a placemarker, or for the case of __VA_OPT__(),
> which is similar to that, in both cases we need to take into account in
> those cases both flags from __VA_OPT__ and from the closing ).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> This is just a partial fix, one thing this patch doesn't change is that
> the standard says that __VA_OPT__ ( contents ) should be treated as
> parameter, which means that #__VA_OPT__ ( contents ) should stringify it,
> which we right now reject.  My preprocessor knowledge is too limited to
> handle this right myself, including all the corner cases, e.g. one can have
> #define f(x, ...) #__VA_OPT__(#x x ## x) etc..  I presume
> m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE);
> could be changed into:
> m_flags = token->flags & (PREV_FALLTHROUGH | PREV_WHITE | STRINGIFY_ARG);
> and when handling the PADDING result from update, we could just emit a 
> "" token, but for INCLUDE_FIRST with this we'd need something complex,
> probably a new routine similar to stringify_arg to some extent.
> 
> I've also cross-checked the libcpp implementation with this patch against
> trunk clang which apparently also implements __VA_OPT__ now, on the
> testcases included here the output is the same and on their
> macro_vaopt_expand.cpp testcase, if I remove all tests that test
> #__VA_OPT__ ( contents ) handling which we just reject now, there are still
> some differences:
> $ /usr/src/llvm/obj8/bin/clang++  -E /tmp/macro_vaopt_expand.cpp -std=c++2a > 
> /tmp/1
> $ ~/src/gcc/obj20/gcc/cc1plus -quiet -E /tmp/macro_vaopt_expand.cpp 
> -std=c++2a > /tmp/2
> diff -up /tmp/1 /tmp/2
> -4: f(0 )
> +4: f(0)
> -6: f(0, a )
> -7: f(0, a )
> +6: f(0, a)
> +7: f(0, a)
> -9: TONG C ( ) B ( ) "A()"
> +9: HT_A() C ( ) B ( ) "A()"
> -16: S foo ;
> +16: S foo;
> -26: B1
> -26_1: B1
> +26: B 1
> +26_1: B 1
> -27: B11
> -27_1: BexpandedA0 11
> -28: B11
> +27: B 11
> +27_1: BA0 11
> +28: B 11
> 
> Perhaps some of the whitespace changes aren't significant, but 9:, and
> 2[678]{,_1}: are significantly different.
> 9: is
> #define LPAREN ( 
> #define A() B LPAREN )
> #define B() C LPAREN )
> #define HT_B() TONG
> #define F(x, ...) HT_ ## __VA_OPT__(x x A()  #x)
> 9: F(A(),1)
> 
> Thoughts on what is right and why?
> Similarly for expansion on the last token from __VA_OPT__ when followed
> by ##, like:
> #define m1 (
> #define f16() f17 m1 )
> #define f17() f18 m1 )
> #define f18() m2 m1 )
> #define m3f17() g
> #define f19(x, ...) m3 ## __VA_OPT__(x x f16() #x)
> #define f20(x, ...) __VA_OPT__(x x)##m4()
> #define f21() f17
> #define f17m4() h
> t25 f19 (f16 (), 1);
> t26 f20 (f21 (), 2);
> 
> E.g. 26: is:
> #define F(a,...)  __VA_OPT__(B a ## a) ## 1
> 26: F(,1)
> I really wonder why clang emits B1 in that case, there
> is no ## in between B and a, so those are 2 separate tokens
> separated by whitespace, even when a ## a is a placemarker.
> Does that mean __VA_OPT__ should throw away all the placemarkers
> and return the last non-placemarker token for the ## handling?
> 
> Can somebody please take the rest over?
> 
> BTW, Tom, perhaps you should update your MAINTAINERS entry email address...
> 
> 2018-01-10  Jakub Jelinek  
> 
>   PR preprocessor/83063
>   PR preprocessor/83708
>   * macro.c (enum macro_arg_token_kind): Fix comment typo.
>   (vaopt_state): Add m_flags field, reorder m_last_was_paste before
>   m_state.
>   (vaopt_state::vaopt_state): Adjust for the above changes.
>   (vaopt_state::update_flags): Add INCLUDE_FIRST, INCLUDE_LAST and
>   PADDING.
>   (vaopt_state::update): Add limit argument, update m_flags member,
>   return INCLUDE_FIRST instead