Re: ecpg: issue related to preprocessor directives

Hi All,

When the following ecpg program having preprocessor directives is compiled,
the output produced is not correct.

/* test program */
exec sql define itype 1;

int main(void)
{
exec sql begin declare section;
exec sql ifdef itype;
int var1;
exec sql elif ntype;
numeric var1;
exec sql else;
float var1;
exec sql endif;
exec sql end declare section;
}

Here is the output produced by th ecpg pre-compiler when above program is
compiled:

int main(void)
{
/* exec sql begin declare section */

#line 8 "2.pgc"
int var1 ;

#line 12 "2.pgc"
float var1 ;

/* exec sql end declare section */
#line 14 "2.pgc"
}

As seen from above output, both exec sql ifdef and exec sql else block got
compiled which is wrong. If the above output is further compiled using gcc
compiler, the compilation would fail.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> writes:
> When the following ecpg program having preprocessor directives is compiled,
> the output produced is not correct.
> ...
> As seen from above output, both exec sql ifdef and exec sql else block got
> compiled which is wrong. If the above output is further compiled using gcc
> compiler, the compilation would fail.

Looking at pgc.l, it seems that 'elif' is treated as though it were
'endif' followed by 'ifdef', which of course completely loses the
expected property that a previous successful branch would keep the
elif branch from being expanded.

While this doesn't look terribly hard to fix, I'm a little disturbed
by the fact that the existing semantics seem to date back to 1999
(b57b0e044). We're probably risking breaking existing app code if
we change it. I think we *should* change it, of course, but I'm kind
of inclined not to back-patch.

regards, tom lane

I wrote:
> Looking at pgc.l, it seems that 'elif' is treated as though it were
> 'endif' followed by 'ifdef', which of course completely loses the
> expected property that a previous successful branch would keep the
> elif branch from being expanded.
> While this doesn't look terribly hard to fix, I'm a little disturbed
> by the fact that the existing semantics seem to date back to 1999
> (b57b0e044). We're probably risking breaking existing app code if
> we change it. I think we *should* change it, of course, but I'm kind
> of inclined not to back-patch.

Here's a proposed patch, which also clarifies the documentation,
which seemed a bit confused/misleading to me.

As stated, I'm not sure it's wise to back-patch this aggressively
... but maybe it'd be okay to squeeze it into v13?

regards, tom lane

On Sat, Aug 1, 2020 at 5:36 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> writes:
> > When the following ecpg program having preprocessor directives is compiled,
> > the output produced is not correct.
> > ...
> > As seen from above output, both exec sql ifdef and exec sql else block got
> > compiled which is wrong. If the above output is further compiled using gcc
> > compiler, the compilation would fail.
>
> Looking at pgc.l, it seems that 'elif' is treated as though it were
> 'endif' followed by 'ifdef', which of course completely loses the
> expected property that a previous successful branch would keep the
> elif branch from being expanded.

Yeah, that's right. The point is, while processing the elif branch, we
remove an entry for the previous branch (ifdef, ifndef) from the stack
and push a new entry for the current elif branch. So, if the elif
branch is evaluated to false, the else branch gets automatically
evaluated to true. And as a result of that both ifdef and else branch
gets evaluated to true thereby compiling both (ifdef/ifndef, else)
blocks.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Hi,

Thanks for the patch. I've spent quite some time reviewing it and the
changes look good to me. It looks very neat and is also
crystal-clear.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Sat, Aug 1, 2020 at 8:05 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> I wrote:
> > Looking at pgc.l, it seems that 'elif' is treated as though it were
> > 'endif' followed by 'ifdef', which of course completely loses the
> > expected property that a previous successful branch would keep the
> > elif branch from being expanded.
> > While this doesn't look terribly hard to fix, I'm a little disturbed
> > by the fact that the existing semantics seem to date back to 1999
> > (b57b0e044). We're probably risking breaking existing app code if
> > we change it. I think we *should* change it, of course, but I'm kind
> > of inclined not to back-patch.
>
> Here's a proposed patch, which also clarifies the documentation,
> which seemed a bit confused/misleading to me.
>
> As stated, I'm not sure it's wise to back-patch this aggressively
> ... but maybe it'd be okay to squeeze it into v13?
>
> regards, tom lane
>

> Here's a proposed patch, which also clarifies the documentation,
> which seemed a bit confused/misleading to me.

Thanks Tom.

> As stated, I'm not sure it's wise to back-patch this aggressively
> ... but maybe it'd be okay to squeeze it into v13?

Fully agreed, back-patching sounds like a pretty risky approach. I
guess almost everyone who ran into this, fixed their code to work
correctly by now. I'd also prefer squeezing it into 13, let's not keep
this around for longer.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De
Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org

Michael Meskes <meskes(at)postgresql(dot)org> writes:
>> As stated, I'm not sure it's wise to back-patch this aggressively
>> ... but maybe it'd be okay to squeeze it into v13?

> Fully agreed, back-patching sounds like a pretty risky approach. I
> guess almost everyone who ran into this, fixed their code to work
> correctly by now. I'd also prefer squeezing it into 13, let's not keep
> this around for longer.

Sounds good, done that way.

regards, tom lane