RFR: 8207345: AArch64: Trampoline generation code reads from uninitialized memory

[Andrew Haley]

On 07/16/2018 05:19 PM, Aleksey Shipilev wrote:
> Looks good, mostly stylistic comments. Your call whether to change any of it:
> 
> *) Let's make NULL checks explicit:
>  748     bool is_c2 = task && is_c2_compile(task->comp_level());

That is explicit, isn't it?  I guess you mean to spell out task != NULL ?
(I always thought that was an advantage of C++ over Java!)

> *) Also, seems useless to break the line here:
>  749     bool in_scratch_emit_size
>  750       = is_c2 && Compile::current()->in_scratch_emit_size();

It'd be way over the (vague) 80 columns limit otherwise.

> *) Also, space after "!":
>  751     if (! in_scratch_emit_size) {

That space makes the negation easier to see, surely?

> *) Also, comment starts with "First, ...", and there is no "Second".
> 
> *) Maybe the whole thing does not need local variables, which would make the chained condition
> clearer at expense of some code duplication.
> 
> In short, that's what I would do:
> 
>    // We need a trampoline if branches are far.
>    if (far_branches()) {
>      // We don't want to emit a trampoline if C2 is generating dummy
>      // code during its branch shortening phase.
>      if (ciEnv::current()->task() != NULL &&
>          is_c2_compile(ciEnv::current()->task()->comp_level()) &&
>          Compile::current->in_scratch_emit_size()) {
>        address stub = emit_trampoline_stub(offset(), entry.target());
>        if (stub == NULL) {
>          return NULL; // CodeCache is full
>        }
>      }
>    }

Hmm.  Maybe I can compromise there somewhere.  That's more or less what
I started with, but I changed it because it was too complex a conditional...

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671