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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
30ede36
8271569: Rename cdsoffsets.cpp to cdsConstants.cpp
Sep 9, 2021
7bb5ee2
Make array of offsets and constants member of class CDSConstants and …
Sep 13, 2021
c290e7c
Rollled back _base_region_crc to private, changed CDSConst's value fr…
Sep 13, 2021
4f045e3
offsets and constants should be private
Sep 14, 2021
5eaf079
remove including <cstddef>, instead include globalDefinitions.hpp exp…
Sep 14, 2021
dfd6525
Reorder friend classes
Sep 14, 2021
10ef5a4
Add back <cstddef> for additional platforms
Sep 14, 2021
d23e70e
Include stddef.h in cds.h
Sep 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 19 additions & 24 deletions src/hotspot/share/cds/cdsConstants.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,14 @@
*/

#include "precompiled.hpp"

#include <cstddef>
Copy link
Member

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

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 •
edited
Loading

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

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

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

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

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

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[] = {
CDSConst CDSConstants::offsets[] = {
{ "CDSFileMapHeaderBase::_magic", offset_of(CDSFileMapHeaderBase, _magic) },
{ "CDSFileMapHeaderBase::_crc", offset_of(CDSFileMapHeaderBase, _crc) },
{ "CDSFileMapHeaderBase::_version", offset_of(CDSFileMapHeaderBase, _version) },
Expand All @@ -44,32 +42,29 @@ struct {
{ "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) },
{ "static_file_header_size", sizeof(FileMapHeader) },
{ "dynamic_archive_header_size", sizeof(DynamicArchiveHeader) },
{ "size_t_size", sizeof(size_t) }
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 get_cds_offset(const char* name) {
for (int i = 0; i < (int)(sizeof(cds_offsets)/sizeof(cds_offsets[0])); i++) {
if (strcmp(name, cds_offsets[i]._name) == 0) {
return cds_offsets[i]._size;
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 (size_t)-1;
return -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;
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;
Expand Down
16 changes: 14 additions & 2 deletions src/hotspot/share/cds/cdsConstants.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,18 @@

#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);

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

class CDSConstants : AllStatic {
public:
static CDSConst offsets[];
static CDSConst constants[];
yminqi marked this conversation as resolved.
Show resolved Hide resolved
static size_t get_cds_constant(const char* name);
static size_t get_cds_offset(const char* name);
};
#endif // SHARE_CDS_CDSCONSTANTS_HPP
yminqi marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions src/hotspot/share/cds/dynamicArchive.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@
#if INCLUDE_CDS

class DynamicArchiveHeader : public FileMapHeader {
friend class CDSConstants;
private:
int _base_header_crc;

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

public:
int base_header_crc() const { return _base_header_crc; }
int base_region_crc(int i) const {
assert(is_valid_region(i), "must be");
Expand Down
3 changes: 1 addition & 2 deletions src/hotspot/share/cds/filemap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ class FileMapRegion: private CDSFileMapRegion {

class FileMapHeader: private CDSFileMapHeaderBase {
friend class VMStructs;
friend class CDSConstants;
Copy link
Member

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;
Expand Down Expand Up @@ -208,11 +209,9 @@ 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:
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
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/prims/whitebox.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2019,14 +2019,14 @@ WB_END
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));
jint result = (jint)get_cds_offset(c_name);
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)get_cds_constant(c_name);
jint result = (jint)CDSConstants::get_cds_constant(c_name);
return result;
WB_END

Expand Down