RFR: 8271471: [IR Framework] Rare occurrence of "<!-- safepoint while printing -->" in PrintIdeal/PrintOptoAssembly can let tests fail

[Christian Hagedorn]

On Thu, 29 Jul 2021 12:25:27 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> A test VM used by the IR framework sometimes prints `` in the middle of emitting a `PrintIdeal` or `PrintOptoAssembly` output which could lead to IR matching failures:
> https://github.com/openjdk/jdk/blob/489e5fd12a37a45f4f5ea64b05f85c6f99f70811/src/hotspot/share/utilities/ostream.cpp#L918-L927
> 
> I thought about just bailing out of IR matching if this string is found after a failure but this issue also affects internal framework tests (I observed one case locally where this happened in the test `TestIRMatching`, letting it fail).
> 
> Handling `` makes things more complicated for the IR framework tests. I'm not sure about the value of printing this message in the first place. But if nobody objects, I suggest to either remove it or at least guard it with `Verbose`, for example. I went with the latter solution in this PR.
> 
> Thanks,
> Christian

I can think of a few options:
1. Keep matching code but if we catch an `IRViolationException` we additionally check if this tty message was found on the matched output. If that is the case, we bail out without an exception:
  - +: Easy and works well for normal user written IR tests. We only need to do this tty if there is actually an IR failure being reported. I'd say that it is unlikely that an IR test is written and is not executed a single time successfully without a bailout before integrating it.
  - -: Does not work well with framework internal tests which sometimes expect an exception (to check a few things on it) and sometimes not. Each test somehow needs to check if there was a bailout to reliably work. I guess it is okay to just ignore it for the "no exception expected" cases. We would catch possible errors in the framework the next time. For the "exception expected" tests, it's a little more difficult. The most simple thing to do is probably to just repeat a single test or a group of tests in an internal test file.
2. Change the regex matching code to ignore the tty message:
  - +: No changes to internal tests.
  - -: Given that it occurs very rarely, it has quite an overhead.
3. Strip a tty message when reading the `PrintIdeal`/`PrintOptoAssembly` entries from the log file when setting up the output to be parsed:
  - +: No changes to internal tests.
  - -: Given that it occurs very rarely, it has some overhead but probably less than the second option. We are reading each line anyways (I'm not actually parsing XML).

In terms of performance, it is probably best to bail out and do a one time effort to make the internal framework tests aware of this bailout scenario (option 1). While thinking about it for a second time, it might not be that complicated as I thought before. My original idea was just to first propose a removal of this tty message before going into fixing this rare case. But of course, given that there are concerns, I'm considering one of these options (my favorite is option 1).

-------------

PR: https://git.openjdk.java.net/jdk/pull/4932