RFR [S] 8081406: cleanup and minor extensions of the debugging facilities in CodeStrings
Bertrand Delsart
bertrand.delsart at oracle.com
Wed Jun 24 09:46:18 UTC 2015
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.
>>>>
>>
>>
--
Bertrand Delsart, Grenoble Engineering Center
Oracle, 180 av. de l'Europe, ZIRST de Montbonnot
38330 Montbonnot Saint Martin, FRANCE
bertrand.delsart at oracle.com Phone : +33 4 76 18 81 23
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
More information about the hotspot-dev
mailing list