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

[David Holmes]

On 17/06/2015 9:07 PM, Bertrand Delsart wrote:
> Thanks David,
>
> Updated webrev. See below my inlined comments for additional
> explanations on the changes.
>
> http://cr.openjdk.java.net/~bdelsart/8081406/webrev.01/
>
> This is a subset of the previous changes:
> - free was reverted to its initial definition but its behavior
>    is now clearly documented (e.g. the fact that it must invalidate
>    the object since this was David's concern)

Okay. I guess there's no point discussing things no longer changed, so 
as long as this works you.

> - assign is nearly reverted too but still ensures _defunct is set

Yep - sorry I got my _defunct comment completely backwards. Doh.

Looks good.

Thanks,
David


> Regards,
>
> Bertrand.
>
> On 17/06/2015 03:42, David Holmes wrote:
>> 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
>
> OK.
>
> [ Will in fact just be "invalidate this" due to your other feedback ]
>
>>
>> ---
>>
>> 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() ).
>
> First, the fields declared in CodeStrings depends on several ifdefs
> (ASSERT and PRODUCT). "*this=" is a way to avoid the "awful ifdef style"
> :-)
>
> In addition, this is slightly more robust IMHO. For instance, it makes
> the assignment copy independent of the internals of CodeStrings. In
> particular, _defunct would have to be set to false, not true :-)
>
> Now, I did check that this had not lead to issue in JPRT and with other
> test in debug mode, including on exotic platforms, but it is impossible
> to prove that all C++ compilers will handle correctly "*this=" which
> should just be an assignment copy between two C++ objects (and a NOP in
> product mode, where the CodeStrings are of size 0).
>
> Since this RFR is blocking later pushes, I'll avoid further delays and
> replace
>    *this = other
> by :
>    _strings = other._strings
> #ifdef ASSERT
>    _defunct = false;
> #endif
>
>> 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 ??
>
> I see your concern about free(). To make that clearer, I added
> explicitly next to the declaration of free the fact that it invalidates
> the buffer.
>
> My change was initially due to closed code in JDK-8087333 (an RFE
> concurrently being reviewed). Extending the meaning of _valid had
> allowed the closed code to be cleaner and more robust, catching errors
> in the handling on non-comment strings (see below) but this may no
> longer be necessary. I'll look for an alternate solution if needed (for
> instance adding a new clear() method; thanks for the suggestion).
>
> Reverting free() in the current webrev means I'll also remove from
> CodeStrings::assign the
>
> +  if (!is_null()) {
> +    // The assignment replaces the old content.
> +    // Delete old CodeStrings, invalidating it if there are non-comment
> +    // strings.
> +    free();
> +    check_valid(); // else the container may reference freed strings
> +  }
>
> and add back the
>
> -  assert(is_null(), "Cannot assign onto non-empty CodeStrings");
>
> FYI, some information on the comment versus non-comment.
>
> Non-comment strings in CodeStrings are (currently) used only in for
> debug functionalities (verify_oop, unimplemented, untested, ...) but
> they are necessary to correctly report errors. If deleted, the VM may
> crash when trying to print a debug message. Since CodeStrings are
> embedded into a CodeBlob container, marking the CodeStrings invalid when
> freed allowed to detect that the CodeBlob was no longer valid. On the
> other hand, a comment string is something which is not necessary for
> correct execution. It is only used when dumping the code.
>
> Removing a comment from a CodeStrings has no impact on the execution of
> the Java application... and freeing them was the most efficient way to
> handle them for JDK-8087333. My change allowed to detect as a side
> effect of the free whether we had safely deleted only comments or
> whether there was a non-comment string that had not been properly
> handled. As stated above, I'll look for an alternative to provide the
> same robustness without impacting the existing methods, avoiding all risks.
>
> Regards,
>
> Bertrand.
>
>>
>> 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.
>>>
>
>