On 1/04/2021 4:19 pm, Ioi Lam wrote:
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.
I don't agree with either of those claims. This is about semantics not
implementation. diagnostic/manageable/experimental are distinct kinds of
product flags with different levels of "support" based on their intended
use in the product. We don't need such distinctions with non-product
flags because the level of "support" is all the same and all flags are
equal.
David
-----
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