JDK-8259843: initialize dli_fname array before calling dll_address_to_library_name

[David Holmes]

Hi Lutz,

On 20/01/2021 11:40 pm, Schmidt, Lutz wrote:
> Hi David,
> my comment was motivated by the "don't fail if you don't have to" principle. You will certainly agree, such principle is very helpful in productive environments. In this particular case, there is no need to fail - just don't print. On the other hand, if these checks describe a "cannot occur situation" - why then checking for it at all?

The condition indicates a programming error in the caller of this API 
hence an assertion would be appropriate. We usually introduce 
release-build checks, after an assert, for cases where we should not get 
the condition (as it indicates a programming error in the VM which we 
expect testing to catch) but there is a possibility of it actually 
happening at runtime due to "external inputs". That is not the case here.

> Anyway, life has gone on since I wrote my comment. Everybody else likes the path you (and Thomas) suggested. I will therefore mute myself. My LGTM statement remains valid.

The original code with just the return is what was committed. :)

Cheers,
David

> Best Regards,
> Lutz
> 
> On 20.01.21, 13:20, "David Holmes" <david.holmes at oracle.com> wrote:
> 
>      On 20/01/2021 6:32 pm, Lutz Schmidt wrote:
>      > On Wed, 20 Jan 2021 08:21:36 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:
>      >
>      >>> src/hotspot/share/runtime/frame.cpp line 543:
>      >>>
>      >>>> 541:   bool found;
>      >>>> 542:
>      >>>> 543:   if (buf == NULL || buflen < 1) return;
>      >>>
>      >>> Can this not just be an assert: buf != NULL && buflen > 0 ?
>      >>
>      >> Hi David,  I think a return would be clearer but an assert is "better than nothing" .
>      >>
>      >> Best regards, Matthias
>      >
>      > With an assert, you assume this is a "cannot occur error". You should be pretty sure to have good test coverage to find all "illegal invocations" before letting a release build escape into the wild.
> 
>      The problem with a return is that it implies these conditions are
>      allowable and could reasonably happen, when in fact it would be an
>      internal programming error in the VM. Do we have test coverage for all
>      the code paths involved? Probably not. But the vast majority of
>      assertions in the VM do not have a "release" bail-out path. Keeping one
>      here is unnecessary and overkill IMO but I won't insist it be removed as
>      this is essentially error reporting code anyway.
> 
>      David
> 
>      > -------------
>      >
>      > PR: https://git.openjdk.java.net/jdk/pull/2144
>      >
>