RFR [S] 8081406: cleanup and minor extensions of the debugging facilities in CodeStrings

David Holmes david.holmes at oracle.com
Wed Jun 17 01:42:44 UTC 2015


Hi Bertrand,

On 29/05/2015 3:02 AM, Bertrand Delsart wrote:
> Hi all,
>
> Small RFR to address minor issues in debug mode with CodeStrings

Not something I'm familiar with so I may be missing some things here.

> https://bugs.openjdk.java.net/browse/JDK-8081406
> http://cr.openjdk.java.net/~bdelsart/8081406/webrev.00/
>
> The change does not impact the product mode.

So the initialization of CodeBlob::_strings and 
CodeBuffer::_code_strings does happen in product mode as well, but the 
resulting CodeStrings object does nothing. Suggests to me that *_strings 
should be a non-product field only (but that's an aside to this set of 
changes).

src/share/vm/asm/codeBuffer.hpp

The ifdef style in this file is quite awful IMO but the changes are 
consistent with it.

308   // FREE strings; invalidate if contains non comment strings.

if -> if it

---

src/share/vm/asm/codeBuffer.cpp

1105   *this = other; // copy all fields (including _defunct when it exists)

I can see the need to free() the existing CodeStrings if present, but 
otherwise why does the original logic need to change from:

   _strings = other._strings;
   _other.set_null_and_invalidate();

to the somewhat cryptic:

  *this = other;
  other.set_null_and_invalidate();

I'm not even certain what that assignment will do. It's also not clear 
why _defunct needs to be copied over rather than just being set to true 
(as _other can't have _defunct==false as we already called check_valid() ).

The change of semantics of free() does make me concerned whether 
existing uses of it now have their assumptions invalidated? It isn't 
clear to me why you would want to free() something but retain validity - 
more of a clear()/reset() operation. But then I don't understand the 
significance of comment versus non-comment with regards to validity ??

Thanks,
David

> In non product mode, CodeStrings allows to associate a linked list of
> strings to a CodeBuffer, CodeBlob or Stub. In addition, with ASSERTS, it
> defines a boolean asserting whether the list of strings are valid.



> Here are the issues addressed by this CR:
>
> - The old code mentioned the fact that CodeStrings was not always
> correctly initialized. This is addressed by the fix, allowing
> check_valid to be added at a few locations where it could currently
> failed due to uninitialized values (like at the beginning of
> CodeStrings::free). This also makes the code more robust against future
> versions of CodeStrings.
>
> - As a minor extension, it is now possible for platform dependent code
> to modify the comment separator used by print_block_comment, which was
> hard coded to " ;; ".
>
> - As another minor extension, related to the validity assertions,
> freeing a code string no longer necessarily marks it (and hence its
> Stub/CodeBlob/CodeBuffer) as invalid. If CodeStrings contains only
> comment, removing them does not change the validity of the CodeStrings.
> For similar reason, assignment over a non null CodeStrings is now valid
> when we can safely free the old string.
>
> The modified code passes JPRT. It was also validated in fastdebug mode
> with the vm.compiler.testlist to check that the validity assertion were
> not triggered. One of our closed extensions also validated advanced use
> of CodeStrings::assign (included cases where the target of the
> assignment was not free).
>
> Best regards,
>
> Bertrand.
>


More information about the hotspot-dev mailing list