[Vm-dev] TMethod>>outputConditionalDefineFor:on: proposed modification

[Alistair Grant]

Hi Eliot,

Thanks for your reply, it made me dive in to the issue in much more depth.


On 22 March 2018 at 20:28, Eliot Miranda <eliot.miranda at gmail.com> wrote:
>
> Hi Alistair,
>
> On Thu, Mar 22, 2018 at 11:32 AM, Alistair Grant <akgrant0710 at gmail.com> wrote:
>>
>>
>> Hi Everyone,
>>
>> VMMaker plugin C generation allows the option: pragma to be used to
>> conditionally compile methods in specific VMs, e.g.:
>>
>>
>> primitiveForPharoOnWin32
>>     <option: #_WIN32>
>>     <option: #PharoVM>
>>
>>     ^self doStuff
>>
>>
>>
>> will generate C code along the lines of:
>>
>> #if _WIN32 && PharoVM
>> sqInt primitiveForPharoOnWin32()
>> { ... }
>> #endif
>>
>>
>> The problem is that on non Win32 platforms this will cause a compiler
>> error as _WIN32 isn't defined, and the resulting directive is invalid.
>>
>> Modifying TMethod>>outputConditionalDefineFor:on: to generate:
>>
>> #if defined(_WIN32) && defined(PharoVM)
>> sqInt primitiveForPharoOnWin32()
>> { ... }
>> #endif
>>
>>
>> avoids the problem.
>
>
> Alas no; it's wrong.  If -DPharoVM=0 then defined(PharoVM) is still true.  I would rather see
>
> primitiveForPharoOnWin32
>     <option: #_WIN32>
>     <option: #PharoVM>
>
>     ^self doStuff
>
> =>
>
> #if _WIN32
> # if PharoVM
> sqInt primitiveForPharoOnWin32()
> { ... }
> # endif /* PharoVM */
> #endif /* _WIN32 */

This will be easy to generate (but see below).


> which should work: (C99, 6.10.1p4) "After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers (including those lexically identical to keywords) are replaced with the pp-number 0"  So in fact, that #if _WIN32 && PharoVM causes an error is a bug (or is it a side effect of _WIN32 not being considered a keyword?), but we have to live in a buggy world.

The key text above is "After all replacements due to macro expansion".

The following example code demonstrates the issue:

$ cat a.c
#include <stdio.h>

#define Macro1

int main()
{

#if Macro1 && Macro2
    printf("True\n");
#endif
    printf("Done\n");
}


$ gcc a.c
a.c: In function ‘main’:
a.c:8:12: error: operator '&&' has no left operand
 #if Macro1 && Macro2


If the #define was absent, and instead we compiled with "gcc -DMacro1
a.c", everything would work since "-DMacro1" is actually short for
"-DMacro1=1".

I hit this issue because I had "<option: UNIX>" as one of the pragmas,
and the UNIX macro is defined in sqConfig.h.



>>
>> Any comment or opposition to me submitting this change?
>
>
> If you can generate and test the nested solution I outlined then feel free.  BTW, I *love* to see
>
> #if FIRSTLEVEL
> #<space>if SECONDLEVEL
> #<tab>if THIRDLEVEL
> #<tab><space><space>if FOURTHLEVEL
>
> etc.  So feel free to add a (dynamic or inst var in CCodeGenerator). variable maintaining the nesting.  VMMaker.oscog assumes tabs are every 4 spaces. This should be written down and ideally added as editor metadata to the source files.  But I may not, and perhaps should not, be able to get my own way on this.

Indenting the code bracketed by the conditionals will be significantly
more effort.  I'm not familiar with editor metadata, and I suspect
this would be critical unless we always use spaces for indenting. (but
see below)



> But if adding
>
> #if !defined(_WIN32)
> # define _WIN32 0
> #endif
>
> somewhere that might be better.

I typed this in without understanding the difference between #if's
behaviour with an undefined macro and an empty macro.  Modifying
platforms/unix/vm/sqConfig.h to use

#define UNIX 1

solves the issue.


>  Alas if falls foul of the same defined(_WIN32) trap.  I wonder, does
>
> #if defined(_WIN32) && defined(PharoVM) && (_WIN32 && PharoVM)
> sqInt primitiveForPharoOnWin32()
> { ... }
> #endif
>
> work?

No, the last expression would still be missing a left or right operand.



>  And when you say it doesn't work, on which platform/compiler combinations are you talking of?

Ubuntu 16.04
gcc 5.4.0, or
gcc 6.3.0


So, after all that...

I think your suggestion of modifying platforms/unix/vm/sqConfig.h so that:

#define UNIX 1

is probably the best solution.



Cheers,
Alistair