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

[David Holmes]

Still seems fine to me.

Thanks,
David

On 24/06/2015 7:46 PM, Bertrand Delsart wrote:
> Hi all,
>
> To avoid further delays since this is a pre-requisite for the closed
> part of 8087333, I've removed the two private accessors I had added:
> set_null() and invalidate().
>
> They are no longer necessary after the cleanup suggested by David and
> their potential added value is currently not worth the concerns around
> their proper use :-)
>
> Updated webrev:
>
> http://cr.openjdk.java.net/~bdelsart/8081406/webrev.02/
>
> Incremental diff:
>
> diff --git a/src/share/vm/asm/codeBuffer.hpp
> b/src/share/vm/asm/codeBuffer.hpp
> --- a/src/share/vm/asm/codeBuffer.hpp
> +++ b/src/share/vm/asm/codeBuffer.hpp
> @@ -264,21 +264,6 @@
>   #endif
>     }
>
> -  void set_null() {
> -#ifndef PRODUCT
> -    _strings = NULL;
> -#endif
> -  }
> -
> -  void invalidate() {
> -#ifndef PRODUCT
> -    assert(is_null(), "Should not invalidate non-empty CodeStrings");
> -#ifdef ASSERT
> -    _defunct = true;
> -#endif
> -#endif
> -  }
> -
>   public:
>     CodeStrings() {
>   #ifndef PRODUCT
>
> Regards,
>
> Bertrand.
>
>
> On 18/06/2015 06:31, David Holmes wrote:
>> 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.
>>>>>
>>>
>>>
>
>