On 3/31/21 10:47 PM, David Holmes wrote:
On 1/04/2021 3:29 pm, Ioi Lam wrote:
On 3/31/21 10:22 PM, David Holmes wrote:
On 1/04/2021 5:05 am, Ioi Lam wrote:
On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore
<cole...@openjdk.org> wrote:
36: // have any MANAGEABLE flags of the ccstr type, but we really
need to
37: // make sure the implementation is correct (in terms of
memory allocation)
38: // just in case someone may add such a flag in the future.
Could you have just added a develop flag to the manageable flags
instead?
I had to use a `product` flag due to the following code, which
should have been removed as part of
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208),
but I was afraid to do so because I didn't have a test case. I.e.,
all of our diagnostic/manageable/experimental flags were `product`
flags.
With this PR, now I have a test case -- I changed
`DummyManageableStringFlag` to a `notproduct` flag, and removed the
following code. I am re-running tiers1-4 now.
Sorry but I don't understand how a test involving one flag replaces
a chunk of code that validated every flag declaration ??
When I did JDK-8243208, I wasn't sure if the VM would actually
support diagnostic/manageable/experimental flags that were not
`product`. So I added check_all_flag_declarations() to prevent anyone
from adding such a flag "casually" without thinking about.
Now that I have added such a flag, and I believe I have tested pretty
thoroughly (tiers 1-4), I think the VM indeed supports these flags.
So now I feel it's safe to remove check_all_flag_declarations().
But the check was checking two things:
1. That diagnostic/manageable/experimental are mutually exclusive
2. That they only apply to product flags
I believe both of these rules still stand based on what we defined
such attributes to mean. And even if you think #2 should not apply, #1
still does and so could still be checked. And if #2 is no longer our
rule then the documentation in globals.hpp should be updated - though
IMHO #2 should remain as-is.
I think neither #1 and #2 make sense. These were limitation introduced
by the old flags implementation, where you had to declare a flag using
one of these macros
diagnostic(type, name, ....
manageable(type, name, ....
experimental(type, name, ....
That's why you have #1 (mutual exclusion).
#2 was also due to the implementation. It makes no sense that you can't
have an develop flag for an experimental feature.
However, in the old implementation, adding more variations would cause
macro explosion. See
https://github.com/openjdk/jdk/blame/7d8519fffe46b6b5139b3aa51b18fcf0249a9e14/src/hotspot/share/runtime/flags/jvmFlag.cpp#L776
Anyway, changing this should be done in a separate RFE. I have reverted
[v2] from this PR, so we are back to [v1].
Thanks
- Ioi
David
-----
Thanks
- Ioi
David
-----
void JVMFlag::check_all_flag_declarations() {
for (JVMFlag* current = &flagTable[0]; current->_name != NULL;
current++) {
int flags = static_cast<int>(current->_flags);
// Backwards compatibility. This will be relaxed/removed in
JDK-7123237.
int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE
| JVMFlag::KIND_EXPERIMENTAL;
if ((flags & mask) != 0) {
assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
(flags & mask) == JVMFlag::KIND_MANAGEABLE ||
(flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
"%s can be declared with at most one of "
"DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL",
current->_name);
assert((flags & KIND_NOT_PRODUCT) == 0 &&
(flags & KIND_DEVELOP) == 0,
"%s has an optional DIAGNOSTIC, MANAGEABLE or
EXPERIMENTAL "
"attribute; it must be declared as a product flag",
current->_name);
}
}
}
-------------
PR: https://git.openjdk.java.net/jdk/pull/3254
