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

[Andrew Dinn]

Hello Dmitrij,

On 09/01/2019 14:50, Dmitrij Pochepko wrote:
> here is my version of this patch consisting of single "sub" instruction
> (I haven't changed test):
> http://cr.openjdk.java.net/~dpochepk/8215792/webrev.01/
> 
> cnt2 is a counter for characters yet to be checked. So, instead of
> checking all characters in source string for first character match
> (which was initial reason for this bug), now it check (str2len - str1len
> + 1).

That looks like a simpler fix than Pengfei's although I think his is
also correct. However, when I say 'correct' note that I can only make
that judgement relative to this current bug. I have no confidence that
there are no other bugs in your code.

> Actually I think this "sub" instruction was initially lost while working
> on this instrinsic and moving this instruction between this block
> (generate_string_indexof_linear) and caller code. Regular tests couldn't
> catch this problem.

That's a somewhat contentious and, I would suggest, dubious statement.
If you design code based on some algorithm -- especially a complex one
like the one employed here -- then you need to put at least as much work
into designing tests that check for problems in the encoding of that
algorithm as you put into the code. 'Regular' is rather a weasel word to
use at this point when it is clear that the test provision was not adequate.

Having looked at your code I am at a loss to see how it is accurately
described by the piece of C code -- i.e. the original Boyer-Moore
algorithm -- that sits in macroAssembler_aarch64.cpp and purports to
explain it. As happened with the trig/log code, your code actually
follows an algorithm that is significantly more complex that that C
original. Also, once again, it employs various coding tricks that are
not explained at all. The latter can be understood with study but proper
commenting would make maintenance and bug-fixing much easier and
quicker. This is exactly the same problem and just as major a problem as
it was with the trig/log code for *all* the same reasons.

> I run some testing to ensure regular usecases are not affected and it
> seems fine. Affected testcase and your test pass as well.

'some testing'? I'd really like to have full details of those tests.
Ideally, they should be comprehensive. That really means they should
come with a test plan that identifies all the different possible paths
through the code and provides a measure of the coverage the tests
actually provide that is high enough to instil some confidence in the
testing. There are indeed quite a few such paths (not just in the stubs
but also the intrinsics that cover the small cases) so I would expect
the test plan and test suite to be fairly large. Do you have such a test
plan and suite?

Given your previous lack of success at testing your own code I'm not at
all happy to accept your say so that 'oh, the code is fine'. I'm
currently more inclined to ask you to revert your first patch and go
back to the original Boyer-Moore code we had before you injected this
bug (and who knows what others?).

> btw: now this code is even faster, because less characters will be
> loaded and checked
Well, of course, you could make it even faster by deleting half the
code. If you don't place too much priority on correctness you can
achieve incredible performance.

Unfortunately, speed has to be secondary to correctness. So, you need to
stop concentrating on shaving cycles and concentrate on writing
readable, maintainable code that clearly implements a well-defined
algorithm. Can you provide any credible assurance that this code is
worth keeping? If not then I'd personally recommend reversion of all
your changes. Of course, I'll see what Andrew Haley has to say before
pressing for that action.

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