[foreign] RFR 8219822: Jextract should handle complex macro constants

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Fri Mar 1 16:39:18 UTC 2019 

Another iteration:

http://cr.openjdk.java.net/~mcimadamore/panama/8219822_v5

changes:

* <stdint.h> cannot be relied upon; it ultimately depends on the machine 
having MSVC headers which in our test infra is not guaranteed

* switched to 'unsigned long long' which should be big enough :-)

* fixed comments in annotations

* added some logger support for errors generated by the macro reparsing 
support, just for diagnosing/debuggability

Maurizio

On 01/03/2019 13:42, Sundararajan Athijegannathan wrote:
> Looks good. Nice & clean!
>
> Minor: doc comment is between annotations and type. Don't we put 
> comment before annotations? (NativeNumericConstant and 
> NativeStringConstant)
>
> -Sundar
>
> On 28/02/19, 8:31 PM, Maurizio Cimadamore wrote:
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8219822_v4/
>>
>> This sues <stdint.h> and then 'uintptr_t' instead of long, which is 
>> the right thing to do.
>>
>> Did not notice any adverse performance issue. I also added your extra 
>> test.
>>
>> Maurizio
>>
>> On 28/02/2019 13:43, Maurizio Cimadamore wrote:
>>>
>>> On 28/02/2019 13:01, Jorn Vernee wrote:
>>>> Hi Maurizio,
>>>>
>>>> Some comments:
>>>>
>>>> * For pointers, I thought casting to (long) would truncate the 
>>>> value on Windows, since long is only 32 bits there, but in practice 
>>>> this does not seem to be a problem... Any ways, we should probably 
>>>> make sure that that case keeps working. Here's the test case I used:
>>>
>>> Right, I knew I was doing something dirty - Ideally we want size_t, 
>>> but that depends on stddef.h... I wanted to avoid extra dependencies 
>>> in the reparsed snippet.
>>>
>>> Interesting that it works - I'd say the clang eval API keeps the 
>>> value as a 64-bit value regardless of the cast...
>>>
>>> Maurizio
>>>
>>>>
>>>> ```
>>>> diff -r 8f30f51887d9 
>>>> test/jdk/com/sun/tools/jextract/ConstantsTest.java
>>>> --- a/test/jdk/com/sun/tools/jextract/ConstantsTest.java Thu Feb 28 
>>>> 12:34:49 2019 +0100
>>>> +++ b/test/jdk/com/sun/tools/jextract/ConstantsTest.java Thu Feb 28 
>>>> 13:29:40 2019 +0100
>>>> @@ -101,7 +101,8 @@
>>>>                  { "CHAR_VALUE", int.class, equalsTo(104) }, 
>>>> //integer char constants have type int
>>>>                  { "MULTICHAR_VALUE", int.class, equalsTo(26728) 
>>>> },  //integer char constants have type int
>>>>                  { "BOOL_VALUE", boolean.class, equalsTo(true) },
>>>> -                { "ZERO_PTR", Pointer.class, pointerEqualsTo(0L) }
>>>> +                { "ZERO_PTR", Pointer.class, pointerEqualsTo(0L) },
>>>> +                { "F_PTR", Pointer.class, 
>>>> pointerEqualsTo(0xFFFF_FFFF_FFFF_FFFFL) }
>>>>          };
>>>>      }
>>>>
>>>> diff -r 8f30f51887d9 test/jdk/com/sun/tools/jextract/constants.h
>>>> --- a/test/jdk/com/sun/tools/jextract/constants.h       Thu Feb 28 
>>>> 12:34:49 2019 +0100
>>>> +++ b/test/jdk/com/sun/tools/jextract/constants.h       Thu Feb 28 
>>>> 13:29:40 2019 +0100
>>>> @@ -65,3 +65,4 @@
>>>>  #define SUB SUP + 2 //dependency
>>>>
>>>>  #define ZERO_PTR (void*)0;
>>>> +#define F_PTR (void*) 0xFFFFFFFFFFFFFFFFLL; // all 1s
>>>> ```
>>>>
>>>> * (I know this was already there); Should the MacroParser.toNumber 
>>>> fast path use Long.decode instead, to catch more values?
>>>
>>> I discussed this with Sundar - we could, but in practice ints cover 
>>> like 90% of cases; so there was a desire to keep the surface area of 
>>> the hack small.
>>>
>>> Of course if you have real use cases which, like OpenGL, gives a 5x 
>>> boost when using such an hack, we can revisit!
>>>
>>> Maurizio
>>>
>>>>
>>>> ---
>>>>
>>>>>> So we have a choice here:
>>>>>>
>>>>>> 1) we extract the value of the constant, and we create a plain 
>>>>>> constant field - e.g.
>>>>>>
>>>>>> static final int FOO = 1;
>>>>>>
>>>>>> which will then work as before (as a true compile-time constant)
>>>>>>
>>>>>> 2) we aim for regularity, and we just translate as
>>>>>>
>>>>>> static final int FOO = lib.FOO();
>>>>
>>>> My vote is on 1., and when condy eventually becomes available, 
>>>> switch the more complex constants to that.
>>>>
>>>> ---
>>>>
>>>> Also, I tested this out with the winreg.h case I had, e.g.:
>>>>
>>>> ```
>>>> typedef struct HKEY__ {
>>>>     int unused;
>>>> } *HKEY;
>>>>
>>>> typedef unsigned __int64 ULONG_PTR;
>>>> typedef long LONG;
>>>>
>>>> #define HKEY_LOCAL_MACHINE (( HKEY ) (ULONG_PTR)((LONG)0x80000002) )
>>>> ```
>>>>
>>>> But unfortunately no constant was generated for HKEY_LOCAL_MACHINE. 
>>>> (Note that __int64 is an MSVC extension, maybe that's the problem?)
>>>>
>>>> ---
>>>>
>>>> On a bit of a tangent; I'm wondering if we really need 2 
>>>> annotations for this? Could we instead use one annotation with a 
>>>> `Class carrier` and `String value` instead? i.e. we encode all 
>>>> constant values as Strings in the annotation and then reparse them 
>>>> when spinning the header impl.
>>>>
>>>> Jorn
>>>>
>>>> Maurizio Cimadamore schreef op 2019-02-28 00:19:
>>>>> Hi,
>>>>> here's a new revision where I just slightly improved the 
>>>>> TestConstants
>>>>> test, to be more uniform (instead of passing expected constant 
>>>>> values,
>>>>> the test now passes checker predicates, which is more scalable).
>>>>>
>>>>> Webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8219822_v3/
>>>>>
>>>>> Cheers
>>>>> Maurizio
>>>>>
>>>>>
>>>>> On 27/02/2019 19:02, Maurizio Cimadamore wrote:
>>>>>> Hi,
>>>>>> this patch regularizes support for constant macros (and enum 
>>>>>> constants too).
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8219822_v2/
>>>>>>
>>>>>> The goal of this patch is to stop having jextract generating 
>>>>>> 'default' methods for constants. Instead, two new annotations are 
>>>>>> introduced:
>>>>>>
>>>>>> @NativeNumericConstant
>>>>>> @NativeStringConstant
>>>>>>
>>>>>> The first takes a long value, the second a String value.
>>>>>>
>>>>>> So, given C code like this:
>>>>>>
>>>>>> #define FOO 1
>>>>>>
>>>>>> instead of generating a method like this:
>>>>>>
>>>>>> default int FOO() {
>>>>>>    return 1;
>>>>>> }
>>>>>>
>>>>>> We will now generate this:
>>>>>>
>>>>>> @NativeNumericConstant(1L)
>>>>>> int FOO();
>>>>>>
>>>>>> And then binder will then generate the implementation.
>>>>>>
>>>>>> Decoupling the generation of the constant methods from jextract 
>>>>>> has a big advantage: we can now support complex constant types, 
>>>>>> such as strings (which need to be turned to Pointer<Byte>) and 
>>>>>> pointer constants (e.g. (void*)0). Note that, whenever we need 
>>>>>> some 'constant' pointer, we need to allocate, which means we need 
>>>>>> a scope - which we don't have at extraction time.
>>>>>>
>>>>>> Efficiency-wise, the binder-generated implementation should be as 
>>>>>> good as the jextract one - it just ldc a constant (which can even 
>>>>>> be a pointer constant, via CP patching!) and then returns it.
>>>>>>
>>>>>> In order to support pointer constants of the kind:
>>>>>>
>>>>>> #define PTR (void*)0
>>>>>>
>>>>>> I also tweaked MacroParser, so that now two attempts can be made 
>>>>>> at evaluating the macro:
>>>>>>
>>>>>> 1) a first attempt is made using a snippet like this:
>>>>>>
>>>>>> __auto_type jextract$var = PTR;
>>>>>>
>>>>>> If this fails (and it does fail in this case, as clang cursor 
>>>>>> evaluation API doesn't detect this constant), then:
>>>>>>
>>>>>> 2) we look at the type of the cursor, if the type is a pointer 
>>>>>> type, then we trigger another snippet parse, this time of:
>>>>>>
>>>>>> __auto_type jextract$var = (long)PTR;
>>>>>>
>>>>>> The extra cast will effectively reinterpret the value as a 
>>>>>> numeric constant. Then we'll create a macro whose type is the 
>>>>>> pointer type determined at (1) and whose value is the one 
>>>>>> determined at (2).
>>>>>>
>>>>>>
>>>>>> Note that, since we only execute step (2) when the type of the 
>>>>>> cursor in (1) is a pointer type, we don't end up executing the 
>>>>>> second step very often. I also tried some of the examples, and 
>>>>>> the extraction time is the same as before this patch.
>>>>>>
>>>>>>
>>>>>> There is a slight issue with AsmCodeFactoryExt: ideally, we'd 
>>>>>> like for constant methods to be turned into static constants 
>>>>>> here; and we'd like to initialize such constants to the result of 
>>>>>> the computation of the bound constant method - that is, if the 
>>>>>> library has a method:
>>>>>>
>>>>>> @NativeNumericConstant(1L)
>>>>>> int FOO();
>>>>>>
>>>>>> Then, the static wrapper should have the following static field:
>>>>>>
>>>>>> static final int FOO = lib.FOO();
>>>>>>
>>>>>> However, if we do so, a test will fail, namely LibEnumTest, as 
>>>>>> that test uses the constants of the static wrapper as labels in a 
>>>>>> 'switch' statement; this only works if the constant field has the 
>>>>>> 'ConstantValue' attribute, and that in turn is only possible if 
>>>>>> the constant is 'simple' - using a condy-based ConstantValue 
>>>>>> would be a way out, but that is not something that is supported 
>>>>>> currently, neither by the VM nor by javac.
>>>>>>
>>>>>> So we have a choice here:
>>>>>>
>>>>>> 1) we extract the value of the constant, and we create a plain 
>>>>>> constant field - e.g.
>>>>>>
>>>>>> static final int FOO = 1;
>>>>>>
>>>>>> which will then work as before (as a true compile-time constant)
>>>>>>
>>>>>> 2) we aim for regularity, and we just translate as
>>>>>>
>>>>>> static final int FOO = lib.FOO();
>>>>>>
>>>>>>
>>>>>> The patch I've submitted does (1) for simple numeric constants 
>>>>>> and falls back to (2) for complex constants. I'm open to 
>>>>>> suggestions as to whether it's better to keep this behavior, or 
>>>>>> simplify it by always aiming for (2).
>>>>>>
>>>>>> Maurizio
>>>>>>
>>>>>>
>>>>>>
>>>>>>

More information about the panama-dev mailing list