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

Fri Mar 1 13:42:26 UTC 2019

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
>>>>>
>>>>>
>>>>>
>>>>>