RFR [XS]: 8233013: deallocate memory after using NEW_C_HEAP_ARRAY

[Kim Barrett]

> On Oct 27, 2019, at 11:57 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 28/10/2019 1:35 pm, Kim Barrett wrote:
>>> On Oct 27, 2019, at 6:32 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> I think this is going to be messy of we try to fight the consistency battle in this area. But I can see an argument for putting in the free call if we wanted to allow the code to be more easily examined by code checkers that were looking at correct usage here. So I don't object to this cleanup.
>> I don't think these help any actually useful code checkers (e.g. ones
>> which look at more than a single translation unit), which will see
>> that the code path leads to abort or exit.
> 
> It requires that you have to teach the tool about vm_exit_during_initialization. I don't know if that is a hurdle or not but it seems an unnecessary complexity to have to add.

Any decent (i.e. usable IMO) code checker figures that out automatically.

(A local-to-TU analysis such as a typical compiler might do might
someday be aided by the C++11 [[noreturn]] attribute.)


>> I really dislike adding effectively useless code.  It expands the code
>> surface area for no good reason.  That has various knock-on effects.
>> For example, some of these lines have been modified multiple times
>> over the years as our memory management infrastructure has changed.
> 
> I'm not sure I see your point re "knock-on effects”.

This is useless code.  And yet there have been multiple occasions when
time has been spent (wasted, IMO) updating it.  And it's more code to
read (or glaze over and hopefully not miss something important nearby
as a result).

>> If we really want consistency, I'd remove the solaris-specific useless
>> code. But because of JEP 362, that doesn't seem to me to be worth the
>> effort. Of course, we've probably now spent more effort...
>> The best way to handle this sort of thing would be to capture the
>> temporary buf in an RAII object at the point of construction. That's
>> true in many other places too.
> 
> So if we had been using RAII then this somehow makes the freeing code not useless? The syntax shouldn't make a difference.

The RAII suggestion is to ensure all return paths perform the cleanup.
These vm_exit_xxx calls are not return paths, so no cleanup is needed
on them. And if they were return paths (say the "exiting" function
doesn't always abort/exit for some reason, like our report_vm_error_xxx
functions, though I think there is code that expects/assumes exit
behavior for some of them), the RAII handling would deal, while the
pre-exit-function frees would be wrong.

(Thinking about it some more, I think they were probably useless or
wrong even in a world where we could terminate and cleanup a VM
without terminating the containing process.  But I don't really want
to go down that rabbit hole.)

> I'm fine either way with this change. I still see it as unnecessary but harmless (by my own definition of "harm").

I disagree with the "harmless" characterization, so think the change
shouldn't be made.