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 4 commits
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,71 @@
/*
* 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"

CDSConst CDSConstants::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) }
};

CDSConst CDSConstants::constants[] = {
{ "static_magic", (size_t)CDS_ARCHIVE_MAGIC },
{ "dynamic_magic", (size_t)CDS_DYNAMIC_ARCHIVE_MAGIC },
{ "int_size", sizeof(int) },
{ "CDSFileMapRegion_size", sizeof(CDSFileMapRegion) },
{ "static_file_header_size", sizeof(FileMapHeader) },
{ "dynamic_archive_header_size", sizeof(DynamicArchiveHeader) },
{ "size_t_size", sizeof(size_t) }
};

size_t CDSConstants::get_cds_offset(const char* name) {
for (int i = 0; i < (int)ARRAY_SIZE(offsets); i++) {
if (strcmp(name, offsets[i]._name) == 0) {
return offsets[i]._value;
}
}
return -1;
}

size_t CDSConstants::get_cds_constant(const char* name) {
for (int i = 0; i < (int)ARRAY_SIZE(constants); i++) {
if (strcmp(name, constants[i]._name) == 0) {
return constants[i]._value;
}
}
return -1;
}
@@ -22,26 +22,21 @@
*
*/

#ifndef SHARE_CDS_CDSOFFSETS_HPP
#define SHARE_CDS_CDSOFFSETS_HPP
#ifndef SHARE_CDS_CDSCONSTANTS_HPP
#define SHARE_CDS_CDSCONSTANTS_HPP

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

class CDSOffsets: public CHeapObj<mtInternal> {
class CDSConstants : AllStatic {
private:
char* _name;
int _offset;
CDSOffsets* _next;
static CDSOffsets* _all; // sole list for cds
static CDSConst offsets[];
static CDSConst constants[];
yminqi marked this conversation as resolved.
Show resolved Hide resolved
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);
static size_t get_cds_constant(const char* name);
static size_t get_cds_offset(const char* name);
};

#endif // SHARE_CDS_CDSOFFSETS_HPP
#endif // SHARE_CDS_CDSCONSTANTS_HPP

This file was deleted.

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

class DynamicArchiveHeader : public FileMapHeader {
friend class CDSOffsets;
friend class CDSConstants;
private:
int _base_header_crc;
int _base_region_crc[MetaspaceShared::n_regions];
yminqi marked this conversation as resolved.
Show resolved Hide resolved
@@ -180,9 +180,10 @@ class FileMapRegion: private CDSFileMapRegion {
};

class FileMapHeader: private CDSFileMapHeaderBase {
friend class CDSOffsets;
friend class VMStructs;
friend class CDSConstants;
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.

It's better to sort the classes alphabetically. CDSConstants should come before VMStructs.


private:
size_t _header_size;

// The following fields record the states of the VM during dump time.
@@ -209,7 +210,6 @@ class FileMapHeader: private CDSFileMapHeaderBase {
// will function correctly with this JVM and the bootclasspath it's
// invoked with.
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;

@@ -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)CDSConstants::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)CDSConstants::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},