[foreign] RFR: 8223489: _Atomic types can cause StackOverflowError

Henry Jen henry.jen at oracle.com
Sat May 18 00:06:20 UTC 2019 

Theoretically, we got Type from Cursor from a HeaderFile, and I checked usage of LayoutUtil, seems the Cursor is available from callers, from which we can track back to the TU.

That would allow us to get rid of static registry and have more targeted search in TypeDictionary.

In other word, we can go extra miles to refactoring use of LayoutUtil using Cursor rather than Type(lose of information), and if we don’t need that work-around stays in clang binding, we can eliminate this distaste.

Cheers,
Henry

> On May 17, 2019, at 2:44 PM, Henry Jen <henry.jen at oracle.com> wrote:
> 
> I explained this in the first email, LayoutUtil in static context and we don’t have a way to get the only instance of jextract.
> 
> Also I did this in libclang so that changed in jextract can be same once the libclang is fixed, all we need to do is remove ClangUtils and then we can put back the Reparser mechanism back into jextract.
> 
> We can choose to keep things in jextract, but that means we need to be able to get LayoutUtil access to THE jextract instance. Let me know if you have better ideas.
> 
> Cheers,
> Henry
> 
> 
>> On May 17, 2019, at 1:49 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>> 
>> Hi Henry,
>> thanks for taking another look at this.
>> 
>> It's good that you managed to get it working, and I appreciate the effort to clean up the JNI code.
>> 
>> There are things in the Java code that leave me a bit perplexed - for instance, the fact that code from MacroParser (eval method) was copied outside it, to Parser.
>> 
>> And also the fact that we are using a register mechanism and stashing things into statics, which is bad practice.
>> 
>> My understanding is that, there should be only one TranslationUnit cursor generated by jextract parser, right?
>> 
>> So, in reality when you register a reparser, you will always call register() with the same TU, right?
>> 
>> Which means the only thing changing are the arguments...
>> 
>> Now, in your current impl, note that you are doing nothing in order e.g. to make sure that if there's already some reparser set with given arguments, that is reused - so again, I'm confused as to what is the role of the static cache.
>> 
>> I think what we want is to have a way to pass the reparser to the TypeDictionary, which is something that can be done by simply passing the TU to the TypeDIctionary, after which the dictionary can create its own reparser to do the atomic reparsing thing.
>> 
>> In other words, I don't see the need for having a getValueType() method on the cursor itself - I think the capability of reparsing things and seeing through 'atomic' is a capability of the type dictionary and we should focus on how to get that capability there.
>> 
>> Maurizio
>> 
>> 
>> On 17/05/2019 21:26, Henry Jen wrote:
>>> Oops, the link to web rev
>>> 
>>> http://cr.openjdk.java.net/~henryjen/panama/8223489/1/webrev/
>>> 
>>> Cheers,
>>> Henry
>>> 
>>> 
>>>> On May 17, 2019, at 1:26 PM, Henry Jen <henry.jen at oracle.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> Please review another webrev[1] that use precompile header to do evaluation as suggested by Maurizio, this patch make one instance of Reparser that can be used by both Macro and Atomic type and potentially other use.
>>>> 
>>>> We took in arguments from the origin TU in parser, this is mainly to make sure the precompile header will be using same setting, this helps the c++ mode in StructTest.java which otherwise fail because the mismatch of original TU(with clang option -x c++).
>>>> 
>>>> This has being an issue, but MacroParser currently implemented in a way that just shutoff in case of any failure and it we simply not notice the issue.
>>>> 
>>>> In this patch, I also refactor clang API a bit to move TranslationUnit functions to where it belong, those were in Index because we didn’t expose TranslationUnit in early days, now we have it, we should organize the function accordingly.
>>>> 
>>>> Another change worth noting, is that Index.parse now throw ParsingFailedException if clang parsing failed in some way. This is why jextract was showing
>>>> "WARNING: nothing to generate” instead of error out when use command line like 'jextract -C -include-pch -C test.h.gch client.h'
>>>> 
>>>> Cheers,
>>>> Henry
>>>> 
>>>>> On May 15, 2019, at 8:50 AM, Henry Jen <henry.jen at oracle.com> wrote:
>>>>> 
>>>>> I figured this out, it failed with a different TU, need to catch the exception in a loop.
>>>>> 
>>>>> Cheers,
>>>>> Henry
>>>>> 
>>>>>> On May 14, 2019, at 9:36 PM, Henry Jen <henry.jen at oracle.com> wrote:
>>>>>> 
>>>>>> BTW, I can get away with incomplete type by adding CXTranslationUnit_Incomplete option, but not sure about typedef,
>>>>>> 
>>>>>> java.lang.IllegalArgumentException: Error with snippet: uchar_t var;
>>>>>> /var/folders/nk/wl6yv8l565g17ykv8658hcq80000gn/T/jextract$4453798577315022046.h:1:1: error: unknown type name 'uchar_t'
>>>>>> 
>>>>>> Cheers,
>>>>>> Henry
>>>>>> 
>>>>>> 
>>>>>>> On May 14, 2019, at 9:10 PM, Henry Jen <henry.jen at oracle.com> wrote:
>>>>>>> 
>>>>>>> Use the reparse mechanism as ClangParser seems a good idea, but there are complications using precompile header.
>>>>>>> 
>>>>>>> In the test case I have, the struct SomeTypes is declared in the header file, but the way we creating new TU use the pch doesn’t work:
>>>>>>> 
>>>>>>> java.lang.IllegalArgumentException: Error with snippet: struct SomeTypes var;
>>>>>>> /var/folders/nk/wl6yv8l565g17ykv8658hcq80000gn/T/jextract$17213029476260309473.h:1:18: error: tentative definition has type 'struct SomeTypes' that is never completed
>>>>>>> 
>>>>>>> I like the idea of reparse, but current way is not working(or I haven’t figure it out), and I am not sure if use or clone(save then create) the original TU would have any side effect. At this point, I think my current implementation is more certain with the downside that we don’t have all builtin types, but at this point, that’s all we supported anyway.
>>>>>>> 
>>>>>>> Since this is to work-around current libclang, I expect the better solution is to bundle a patched libclang. But until then, this temporary fix should be good enough.
>>>>>>> 
>>>>>>> Cheers,
>>>>>>> Henry
>>>>>>> 
>>>>>>>> On May 14, 2019, at 4:03 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> I see what you are doing here. I believe there's a cleaner way to get there, which is what we've done to process macros.
>>>>>>>> 
>>>>>>>> That is, when you see a cursor with:
>>>>>>>> 
>>>>>>>> _Atomic("....")
>>>>>>>> 
>>>>>>>> extract the innards of that cursor, and create a new temporary compilation unit for that one, as we do for macros. I think most of the logic we use there should be applicable in this case (only the snippet to be generated for the speculative compilation would be different). And, probably, if we go down that path we could refactor the code a bit so that the functionalities used for the 'speculative' clang evaluation can be shared across macro/atomic support.
>>>>>>>> 
>>>>>>>> Maurizio
>>>>>>>> 
>>>>>>>> On 14/05/2019 02:58, Henry Jen wrote:
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>> Please review a webrev[1] that detects an atomic type to get the correct layout for the type.
>>>>>>>>> 
>>>>>>>>> A proper solution would be have libclang expose the atomic types, so we don’t need to add the ugly hacks in Java to find the type. A patch is submitted[2] to clang project and hopefully this can be fixed in future release.
>>>>>>>>> 
>>>>>>>>> Before that happens, we have our java clang binding trying do that work by:
>>>>>>>>> - Put together a temporary header file with C11 types to parse, so we can get this builtin-types.
>>>>>>>>> - During the cursor-traversing, we will add in extra types declared in the header files
>>>>>>>>> - For an atomic type, we use the type string to get the underlying type.
>>>>>>>>> 
>>>>>>>>> Note that an atomic type is always a builtin type without valid declaration cursor, so we cannot get the translation unit the type actually defined in, so we are doing a global search. Since jextract will only created one translation unit, the hack should work just fine without much pollution.
>>>>>>>>> 
>>>>>>>>> Cheers,
>>>>>>>>> Henry
>>>>>>>>> 
>>>>>>>>> [1] http://cr.openjdk.java.net/~henryjen/panama/8223489/0/webrev/
>>>>>>>>> [2] https://reviews.llvm.org/D61716
>

More information about the panama-dev mailing list