RFR: 8248411: [aarch64] Insufficient error handling when CodeBuffer is exhausted

Andrew Haley aph at openjdk.java.net
Wed Oct 21 12:59:22 UTC 2020


On Wed, 21 Oct 2020 11:31:47 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> Trampoline call generation (in the macro-assembler) may run out of CodeBuffer space without the proper error handling, resulting in asserts such as:
>> #  Internal Error (.../open/src/hotspot/share/asm/codeBuffer.hpp:198), pid=845, tid=859
>> #  assert(allocates2(pc)) failed: relocation addr must be in this section
>> This update extends the error handling for such error cases to cover all uses of `trampoline_call()`, direct and indirect. Failure registration/recording is retained in the "**aarch64.ad**" file.
>
> The AArch64 changes are ok.
> <rant> I am not at all keen on the many format-only changes that are included in this patch since they introduce a lot of changed lines for the sole and rather specious benefit of adherence to a questionable orthographic authority.  That's especially so with the relatively unscryable (sic) changes in output.cpp that modify declarations of the form `Foo *foo` to `Foo* foo`. One is initially left wondering what has changed only, at penny-drop, to replace that feeling with equal wonder as to why it was worth bothering, especially as there remain many thousands more such editorial opportunities. Meanwhile the substantive signal that constitutes the real patch is lost amid this noise. Of course, you may continue tilting at this windmill if you really wish to.</rant>

> The AArch64 changes are ok.
> <rant> I am not at all keen on the many format-only changes that are included in this patch since they introduce a lot of changed lines for the sole and rather specious benefit of adherence to a questionable orthographic authority. That's especially so with the relatively unscryable (sic) changes in output.cpp that modify declarations of the form `Foo *foo` to `Foo* foo`. One is initially left wondering what has changed only, at penny-drop, to replace that feeling with equal wonder as to why it was worth bothering, especially as there remain many thousands more such editorial opportunities. Meanwhile the substantive signal that constitutes the real patch is lost amid this noise. Of course, you may continue tilting at this windmill if you really wish to.</rant>

I agree. I'm happy enough with code being tidied up as we go along, but not with a patch like this one, where it's not even clear that the result is an improvement. Also, it doesn't make sense to "tidy up" code that is nothing to do with the patch. 

Changing  `Foo *foo` to `Foo* foo` is simply wrong. The * operator binds to the right, so something like `int* a, b` looks like a and b are pointers; they're not. That's why we write `int* a, b` : we should be writing for the reader.

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

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


More information about the hotspot-dev mailing list