RFR(S): 8215792: AArch64: String.indexOf generates incorrect result

[Andrew Dinn]

Hello Dmitrij,

On 10/01/2019 15:10, Dmitrij Pochepko wrote:

> I’ll focus on addressing your technical questions about testing this
> patch and intrinsic first.
> . . .
> I referenced this test in initial review request for this intrinsic. It
> takes a long time to run, so I did not include it in the webrev. I'm
> going to update the webrev to include a subset of this test as jtreg.

Ok, thank you for providing full details of the testing regime. If you
add the test as a jtreg test then I'm happy for it and your one line fix
to be pushed.

> Even brute force tests with 100% code coverage don't guarantee 100%
> correctness. The search-garbage-after-string test case for "algorithm G"
> and StringBuilder::setLength usage is a good catch by Stefan and
> Pengfei. And recent webrev addresses this case. I also tested a case
> symmetric to Pengfei's case checking that no "garbage" is read before
> specified source string [4]. I also am going to include it in the webrev.

I am aware of the limits of brute force methods. However, note that in
my previous post I set the bar at tests that would inspire confidence in
the code not ones that would guarantee correctness. God forbid that we
go down the route of formal verification, Grails are hard to come by.

The second, extra jtreg test is good.

> Indeed it is hard to review complex algorithms. The Boyer-Moore comments
> you referenced were updated as part of the original webrev to describe
> changes in algorithm E, which is in macroAssembler_aarch64.cpp. I once
> asked to validate the level of comments with you during pow function
> review [3]. If this is the level of comments you find reasonable, I’ll
> be happy to improve it here and elsewhere to this level.

Yes, I believe the code generated in the stub needs more documentation.
However, it is important to fix what is currently broken quickly. Please
raise a separate JIRA for the doc fixes and then submit an algorithm
and/or comments in the generator code that explain what the stub is doing.

> Once again, this is to address your question around testing for this
> intrinsic and patch. We are working on testing and review complex
> intrinsics to handle the wider problem of ensuring better quality of
> AArch64 intrinsics. We’ll follow up in a different email on that.
Well, one thing that needs to form part of that discussion is the
potential benefit of these patches vs the cost of producing, reviewing
and maintaining them. Included in the equation for the benefits is the
number of users it will help and the criticality of the problem they
face without the patch. On the costs side we need to factor in the
effort needed to clearly document complex code compared with the
potential cost of someone having to pick it up later and also the
potential, even with good documentation, of the resulting code becoming
a fly trap for developer and/or maintainer time.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander