Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271569: Clean up the use of CDS constants and field offsets #5450

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,76 @@
/*
* Copyright (c) 2014, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/

#include "precompiled.hpp"
#include <cstddef>
Copy link
Member

@stefank stefank Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not stated in the style guide, but I think most files include the system headers after the HotSpot headers, separated by a blank line.

Copy link
Member

@dholmes-ora dholmes-ora Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even needed now? I don't see it being used.

Copy link
Member

@stefank stefank Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, the layout that I'd like to see is:

#include "precompiled.hpp"
#include "cds.hpp"
...
#include "cds/filemap.hpp"

#include <cstddef>

However, I think the more appropriate change is to include globalDefinitions.hpp instead of explicitly including cstddef.

Copy link
Contributor Author

@yminqi yminqi Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pre-submit tests failed to build hotspot for linux on arm/ppc/s390 etc after removed. needed for defining size_t. So will add it back at last section of 'include'.

Copy link
Member

@iklam iklam Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is here in cdsConstants.hpp

#include "memory/allStatic.hpp"
typedef struct {
  const char* _name;
  size_t _value;
} CDSConst;

globalDefinitions.hpp should be included here for the declaration of size_t.

Copy link
Contributor Author

@yminqi yminqi Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it still failed for non-pch since cds.h is before cdsConstants.hpp. size_t in cds.h is not defined yet. Should I add it to cds.h?

Copy link
Member

@iklam iklam Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit complicated. The file is src/hotspot/share/include/cds.h, and the HotSpot convention for including files in this directory usually drops the "include/" part. E.g.,

#include "jvm.h"

But this will put #include "cds.h" at the very beginning of all includes, so it doesn't take up size_t which is indirectly declared later via globalDefinitions.hpp.

However, header files in src/hotspot/share/ may be included by C/C++ files outside of HotSpot. As a result, these headers cannot include other HotSpot headers. I.e., you cannot put #include "utilities/globalDefinitions.hpp" in cds.h, because cds.h is included by src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c.

I think one compromise is to add #include <stddefs.h> in cds.h. Since cds.h can be included by C source code, we cannot use #include <cstddef> which is a C++ thing. <stddefs.h> is part of ANSI C standard and we already include it inside share/utilities/debug.hpp, so we can safely assume that it exists for all compilers that can build HotSpot.

Copy link
Member

@dholmes-ora dholmes-ora Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Ioi - use stddefs.h in cds.h

#include "cds.h"
#include "cds/cdsConstants.hpp"
#include "cds/dynamicArchive.hpp"
#include "cds/filemap.hpp"

struct {
const char* _name;
size_t _size;
} cds_offsets[] = {
{ "CDSFileMapHeaderBase::_magic", offset_of(CDSFileMapHeaderBase, _magic) },
{ "CDSFileMapHeaderBase::_crc", offset_of(CDSFileMapHeaderBase, _crc) },
{ "CDSFileMapHeaderBase::_version", offset_of(CDSFileMapHeaderBase, _version) },
{ "CDSFileMapHeaderBase::_space[0]", offset_of(CDSFileMapHeaderBase, _space) },
{ "FileMapHeader::_jvm_ident", offset_of(FileMapHeader, _jvm_ident) },
{ "FileMapHeader::_base_archive_name_size", offset_of(FileMapHeader, _base_archive_name_size) },
{ "CDSFileMapRegion::_crc", offset_of(CDSFileMapRegion, _crc) },
{ "CDSFileMapRegion::_used", offset_of(CDSFileMapRegion, _used) },
{ "DynamicArchiveHeader::_base_region_crc", offset_of(DynamicArchiveHeader, _base_region_crc) }
};

constexpr struct {
const char* _name;
int _value;
} cds_constants[] = {
{ "static_magic", (int)CDS_ARCHIVE_MAGIC },
{ "dynamic_magic", (int)CDS_DYNAMIC_ARCHIVE_MAGIC },
{ "int_size", sizeof(int) },
{ "CDSFileMapRegion_size", sizeof(CDSFileMapRegion) },
Copy link
Member

@iklam iklam Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be (int)sizeof(int) because the type of sizeof(...) is implementation-defined. See https://en.cppreference.com/w/cpp/types/size_t

Copy link
Contributor Author

@yminqi yminqi Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that all almost all (except for 'magic') with size_t, we should use name vs size_t in the struct and get functions, cast when return from vm in whiteBox.cpp.

{ "static_file_header_size", sizeof(FileMapHeader) },
{ "dynamic_archive_header_size", sizeof(DynamicArchiveHeader) },
{ "size_t_size", sizeof(size_t) }
};

size_t get_cds_offset(const char* name) {
for (int i = 0; i < (int)(sizeof(cds_offsets)/sizeof(cds_offsets[0])); i++) {
Copy link
Member

@dholmes-ora dholmes-ora Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just define a size for the array and use that? It isn't really a dynamic/unknown quantity.

Copy link
Contributor Author

@yminqi yminqi Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array size is known at compile time --- it is not dynamic but we do not need to manually count the size of the array this way. If define a size for the array, we need to count the number of items in the array to define the array size. We may add more items to the array --- just add items and don't need change anything else. Since total array memory size and array component size are known at compile time, so compiler will not generate code to calculate the size. How about this?
const int size = (int)(sizeof(cds_offsets)/sizeof(cds_offsets[0]));
for (int i = 0; i < size; i++) {
...
}

Copy link
Member

@dholmes-ora dholmes-ora Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always thought that was a hack way of determining an array size, not an approved method. :(

Don't worry just leave it as-is.

Copy link
Member

@stefank stefank Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have functionality for this in globalDefinitions.hpp:

#define ARRAY_SIZE(array) sizeof(array_size_impl(array))

Copy link
Contributor Author

@yminqi yminqi Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefank Thanks --- it saves code size.

if (strcmp(name, cds_offsets[i]._name) == 0) {
return cds_offsets[i]._size;
}
}
return (size_t)-1;
}

int get_cds_constant(const char* name) {
for (int i = 0; i < (int)(sizeof(cds_constants)/sizeof(cds_constants[0])); i++) {
if (strcmp(name, cds_constants[i]._name) == 0) {
return cds_constants[i]._value;
}
}
return -1;
}
@@ -22,26 +22,8 @@
*
*/

#ifndef SHARE_CDS_CDSOFFSETS_HPP
#define SHARE_CDS_CDSOFFSETS_HPP

#include "memory/allocation.hpp"

class CDSOffsets: public CHeapObj<mtInternal> {
private:
char* _name;
int _offset;
CDSOffsets* _next;
static CDSOffsets* _all; // sole list for cds
public:
CDSOffsets(const char* name, int offset, CDSOffsets* next);

char* get_name() const { return _name; }
int get_offset() const { return _offset; }
CDSOffsets* next() const { return _next; }
void add_end(CDSOffsets* n);

static int find_offset(const char* name);
};

#endif // SHARE_CDS_CDSOFFSETS_HPP
#ifndef SHARE_CDS_CDSCONSTANTS_HPP
#define SHARE_CDS_CDSCONSTANTS_HPP
int get_cds_constant(const char* name);
size_t get_cds_offset(const char* name);
#endif // SHARE_CDS_CDSCONSTANTS_HPP

This file was deleted.

@@ -38,12 +38,12 @@
#if INCLUDE_CDS

class DynamicArchiveHeader : public FileMapHeader {
friend class CDSOffsets;
private:
int _base_header_crc;
int _base_region_crc[MetaspaceShared::n_regions];

public:
int _base_region_crc[MetaspaceShared::n_regions];
yminqi marked this conversation as resolved.
Show resolved Hide resolved

int base_header_crc() const { return _base_header_crc; }
int base_region_crc(int i) const {
assert(is_valid_region(i), "must be");
@@ -180,9 +180,9 @@ class FileMapRegion: private CDSFileMapRegion {
};

class FileMapHeader: private CDSFileMapHeaderBase {
friend class CDSOffsets;
friend class VMStructs;

private:
size_t _header_size;

// The following fields record the states of the VM during dump time.
@@ -208,10 +208,11 @@ class FileMapHeader: private CDSFileMapHeaderBase {
// The following fields are all sanity checks for whether this archive
// will function correctly with this JVM and the bootclasspath it's
// invoked with.
public:
Copy link
Member

@dholmes-ora dholmes-ora Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again this looks like it should be private with either a public accessor or else a friend declaration for the client code.

Copy link
Contributor Author

@yminqi yminqi Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same reason as above.

char _jvm_ident[JVM_IDENT_MAX]; // identifier string of the jvm that created this dump

// size of the base archive name including NULL terminator
size_t _base_archive_name_size;
private:

// The following is a table of all the boot/app/module path entries that were used
// during dumping. At run time, we validate these entries according to their
@@ -24,7 +24,7 @@

#include "precompiled.hpp"
#include <new>
#include "cds/cdsoffsets.hpp"
#include "cds/cdsConstants.hpp"
#include "cds/filemap.hpp"
#include "cds/heapShared.inline.hpp"
#include "cds/metaspaceShared.hpp"
@@ -2016,11 +2016,18 @@ WB_END

#if INCLUDE_CDS

WB_ENTRY(jint, WB_GetOffsetForName(JNIEnv* env, jobject o, jstring name))
WB_ENTRY(jint, WB_GetCDSOffsetForName(JNIEnv* env, jobject o, jstring name))
ResourceMark rm;
char* c_name = java_lang_String::as_utf8_string(JNIHandles::resolve_non_null(name));
int result = CDSOffsets::find_offset(c_name);
return (jint)result;
jint result = (jint)get_cds_offset(c_name);
return result;
WB_END

WB_ENTRY(jint, WB_GetCDSConstantForName(JNIEnv* env, jobject o, jstring name))
ResourceMark rm;
char* c_name = java_lang_String::as_utf8_string(JNIHandles::resolve_non_null(name));
jint result = (jint)get_cds_constant(c_name);
return result;
WB_END

#endif // INCLUDE_CDS
@@ -2457,7 +2464,8 @@ static JNINativeMethod methods[] = {
{CC"readFromNoaccessArea",CC"()V", (void*)&WB_ReadFromNoaccessArea},
{CC"stressVirtualSpaceResize",CC"(JJJ)I", (void*)&WB_StressVirtualSpaceResize},
#if INCLUDE_CDS
{CC"getOffsetForName0", CC"(Ljava/lang/String;)I", (void*)&WB_GetOffsetForName},
{CC"getCDSOffsetForName0", CC"(Ljava/lang/String;)I", (void*)&WB_GetCDSOffsetForName},
{CC"getCDSConstantForName0", CC"(Ljava/lang/String;)I", (void*)&WB_GetCDSConstantForName},
#endif
#if INCLUDE_G1GC
{CC"g1InConcurrentMark", CC"()Z", (void*)&WB_G1InConcurrentMark},