RFR(S): 8247732: validate user-input intrinsic_ids in ControlIntrinsic

[Nils Eliasson]

Hi again,

On second thought - please add some basic testing (reuse any old test, 
or write a new) that covers the different cases.

I think this table covers all combinations. There should exist tests for 
most of them that you can piggy back on.

|+-------------------------------------------------+-------+----------------------------------+ 
| ControlIntrinsics | valid | invalid | 
+-------------------------------------------------+-------+----------------------------------+ 
| vmflag | ok | print error and don't start | 
+-------------------------------------------------+-------+----------------------------------+ 
| CompilerOracle: -XX:CompileCommand= | ok | print error and continue | 
+-------------------------------------------------+-------+----------------------------------+ 
| CompilerDirectives: -XX:CompilerDirectivesFile= | ok | print error and 
don't start | 
+-------------------------------------------------+-------+----------------------------------+ 
| CompilerDirectives via jcmd | ok | print error, vm continues to run | 
+-------------------------------------------------+-------+----------------------------------+|


Regards,
Nils


On 2020-08-12 10:21, Nils Eliasson wrote:
> Hi,
>
> Sorry for the delay.
>
> About the error handling:
>
> For CompilerDirectivesFile there are two scenarios:
> 1) If a file containing bad contents is passed on the commandline - 
> the VM prints an descriptive error and refuses to start.
> 2) If a file containing bad contents is passed through jcmd - the VM 
> prints and error on the jcmd stream and continues to run (ignoring the 
> command).
>
> This is achieved by letting the parser just register any parsing 
> error, and defer to the caller to decide how to handle the situation.
>
> Regards,
> Nils Eliasson
>
>
> On 2020-08-11 19:09, Liu, Xin wrote:
>> Hi, Reviewers,
>>
>> May I gently ping this?
>>
>> I stuck because I don't know which error handling is appropriate.
>>
>> If we do nothing, current hotspot ignores wrong intrinsic Ids in the 
>> cmdline.
>> This patch aborts hotspot when it detects any invalid intrinsic id.
>>
>> thanks,
>> --lx
>>
>>
>> ________________________________________
>> From: Liu, Xin
>> Sent: Monday, August 3, 2020 11:39 PM
>> To: Tobias Hartmann; Nils Eliasson; 
>> hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-dev
>> Subject: Re: [EXTERNAL] RFR(S): 8247732: validate user-input 
>> intrinsic_ids in ControlIntrinsic
>>
>> hi, Nils,
>>
>> Tobias would like to keep the parser behavior consistency.  I think 
>> it means that the hotspot need to suppress the warning if the 
>> intrinsic_id doesn't exists in compiler directive.
>> eg. -XX:CompileCommand=option,<pattern>,ControlIntrinsic=-_nonexist.
>>
>> What do you think about it?
>>
>> Here is the latest webrev:
>> http://cr.openjdk.java.net/~xliu/8247732/01/webrev/
>>
>> thanks,
>> --lx
>>
>> ________________________________________
>> From: Tobias Hartmann <tobias.hartmann at oracle.com>
>> Sent: Friday, July 24, 2020 2:52 AM
>> To: Liu, Xin; Nils Eliasson; hotspot-compiler-dev at openjdk.java.net; 
>> hotspot-runtime-dev
>> Subject: RE: [EXTERNAL] RFR(S): 8247732: validate user-input 
>> intrinsic_ids in ControlIntrinsic
>>
>> CAUTION: This email originated from outside of the organization. Do 
>> not click links or open attachments unless you can confirm the sender 
>> and know the content is safe.
>>
>>
>>
>> Hi Liu,
>>
>> On 23.07.20 18:02, Liu, Xin wrote:
>>> That is my intention too, but CompilerOracle doesn't exit JVM when 
>>> it encounters parsing errors.
>>> It just exacts information from CompileCommand as many as possible. 
>>> That makes sense because compiler "directives" are supposed to be 
>>> optional for program execution.
>>>
>>> I do put the error message in parser's errorbuf.  I set a flag 
>>> "exit_on_error" to quit JVM after it dumps parser errors. yes, I 
>>> treat undefined intrinsics as fatal errors.
>>> This behavior is from Nils comment: "I want to see an error on 
>>> startup if the user has specified unknown intrinsic names." It is 
>>> also consistent with JVM option -XX:ControlIntrinsic=.
>> Okay, thanks for the explanation! I would prefer consistency in error 
>> handling of compiler
>> directives, i.e., handle all parser failures the same way. But I 
>> leave it to Nils to decide.
>>
>> Best regards,
>> Tobias
>