Remove duplicate macros between j9nonbuilder.h and j9consts.h #4287

AidanHa · 2019-01-14T18:23:27Z

-There were various duplicate macros in j9consts.h and j9nonbuilder.h. Since j9nonbuilder.h seemed to be more up to date, I removed several constants from j9consts.h, including the following:

Note: This is the second pull request attempt.

J9_JAVA_PUBLIC
J9_JAVA_PRIVATE
J9_JAVA_PROTECTED 0x4
J9_JAVA_STATIC (CARE IN STACK WALKER.JAVA)
J9_JAVA_FINAL
J9_JAVA_CLASS_RAM_SHAPE_SHIFT
J9_JAVA_CLASS_REFERENCE_SHIFT
J9_JAVA_SYNC
J9_JAVA_NATIVE
J9_JAVA_GETTER_METHOD
J9_JAVA_INTERFACE
J9_JAVA_UNUSED_0X200
J9_JAVA_ABSTRACT
J9_JAVA_FORWARDER_METHOD
J9_JAVA_EMPTY_METHOD
J9_JAVA_CLASS_DEPTH_MASK
J9_JAVA_CLASS_ARRAY
J9_JAVA_CLASS_RAM_ARRAY
J9_JAVA_METHOD_VTABLE
J9_JAVA_METHOD_HAS_EXCEPTIONS
J9_JAVA_CLASS_PRIMITIVE_TYPE
J9_JAVA_CLASS_UNSAFE
J9_JAVA_METHOD_FRAME_ITERATOR_SKIP
J9_JAVA_METHOD_CALLER_SENSITIVE
J9_JAVA_CLASS_BYTECODES_MODIFIED
J9_JAVA_CLASS_OWNABLE_SYNCHRONIZER
J9_JAVA_CLASS_HAS_EMPTY_FINALIZE
J9_JAVA_CLASS_HAS_JDBC_NATIVES
J9_JAVA_METHOD_OBJECT_CONSTRUCTOR
J9_JAVA_CLASS_GC_SPECIAL
J9_JAVA_CLASS_HAS_VERIFY_DATA
J9_JAVA_METHOD_UNUSED_1000000
J9_JSVS_CLASS_CONTENDED
J9_JAVA_CLASS_HAS_FINAL_FIELDS
J9_JAVA_METHOD_HAS_GENERIC_SIGNATURE
J9_JAVA_CLASS_HOT_SWAPPED_OUT
J9_JAVA_CLASS_HAS_CLINIT
J9_JAVA_CLASS_DYING
J9_JAVA_CLASS_REFERENCE_WEAK
J9_JAVA_FIELD_UNUSED_10000000
J9_JAVA_CLASS_REFERENCE_SOFT
J9_JAVA_CLASS_REFERENCE_MASK
J9_JAVA_CLASS_REFERENCE_PHANTOM
J9_JAVA_CLASS_CLONEABLE
J9_JAVA_CLASS_ROMRAMMASK

-I renamed the constants in various files. I did not touch anything in any of the DDR files @tajila
-There were alot of constants to renamed, so hopefully I did not made any typos.
I also added <#include "j9nonbuilder.h"> where necessary.

I also updated the Copyright comments where necessary, and modified the .gitignore file to ignore any .vscode folders

fjeremic

I can't comment on the content of the PR however the commit needs some work, more specifically [1]:

The first line describes the change made. It is written in the imperative mood, and should say what happens when the patch is applied. Keep it short and simple. The first line should be less than 70 characters, where reasonable, and should be written in sentence case preferably not ending in a period. Leave a blank line between the first line and the message body.

There seems to also be a few sign offs on the same commit which is unnecessary. See [1] for examples of commit contents.

[1] https://github.com/eclipse/openj9/blob/master/CONTRIBUTING.md#commit-guidelines

AidanHa · 2019-01-14T19:41:54Z

Ah, thank you for the link @fjeremic I will edit the commit messages.

The multiple unnecessary sign offs come from me squashing multiple smaller commits together. Will I only need to sign off one commit in this case (sign off the latest commit)?

fjeremic · 2019-01-14T20:05:58Z

Will I only need to sign off one commit in this case (sign off the latest commit)?

That's correct.

tajila · 2019-01-22T16:25:05Z

.gitignore

@@ -1,4 +1,4 @@
-# Copyright (c) 2000, 2018 IBM Corp. and others
+# Copyright (c) 2000, 2019 IBM Corp. and others
 #


can you please revert the changes in this file

tajila · 2019-01-22T16:28:09Z

runtime/compiler/env/VMJ9.cpp

@@ -32,6 +32,7 @@
 #include "cache.h"
 #include "fastJNI.h"
 #include "j9cfg.h"
+


please remove this added whitespace

tajila · 2019-01-22T16:28:31Z

runtime/compiler/il/symbol/J9Symbol.cpp

@@ -35,6 +35,7 @@
 #include <stddef.h>                            // for size_t
 #include <stdint.h>                            // for uint32_t, int32_t, etc
 #include <string.h>                            // for memcmp, strncmp
+


tajila · 2019-01-22T16:32:50Z

runtime/compiler/runtime/SymbolValidationManager.hpp

@@ -32,6 +32,7 @@
 #include "env/jittypes.h"                  // for uintptrj_t
 #include "j9.h"
 #include "j9nonbuilder.h"
+#include "j9consts.h"


was this needed?

tajila · 2019-01-22T16:33:54Z

runtime/gc_base/ClassLoaderManager.hpp

@@ -32,7 +32,6 @@
 #include "j9.h"
 #include "j9cfg.h"

-


undo this change

tajila · 2019-01-22T16:35:32Z

runtime/gc_glue_java/ConcurrentMarkingDelegate.hpp

@@ -33,6 +33,7 @@
 #include "j9modron.h"
 #include "j9nongenerated.h"
 #include "j9nonbuilder.h"
+#include "j9consts.h"


what was this needed for?

tajila · 2019-01-22T16:36:15Z

runtime/gc_glue_java/MarkingDelegate.hpp

@@ -25,6 +25,7 @@
 #define MARKINGDELEGATE_HPP_

 #include "j9nonbuilder.h"
+#include "j9consts.h"


tajila · 2019-01-22T16:36:31Z

runtime/gc_glue_java/ObjectModelDelegate.hpp

@@ -24,6 +24,7 @@
 #define OBJECTMODELDELEGATE_HPP_

 #include "j9nonbuilder.h"
+#include "j9consts.h"


tajila · 2019-01-22T16:42:41Z

runtime/oti/j9nonbuilder.h

@@ -33,6 +33,7 @@
 #include "j9vmconstantpool.h"
 #include "j9consts.h"
 #include "jvmti.h"
+#include "omrsrp.h"


is this still needed here, now that the macros are moved over to j9const.h ?

tajila · 2019-01-22T16:44:16Z

runtime/vm/guardedstorage.c

@@ -24,6 +24,7 @@
 #include "j9gs.h"
 #include "j9port_generated.h"
 #include "j9nonbuilder.h"
+#include "j9consts.h"


is this needed?

AidanHa · 2019-01-22T16:50:28Z

@tajila for the some of the #include "j9consts.h" calls, I just wanted to make sure that all files that included nonbuilder.h also had j9consts (kind of a surefire way to make sure there were no errors). I will now remove all the unessesary #includes.

The added whitespace was me accidentally forgetting to remove them. Sorry.

Any reason why the .gitignore file changes should be reverted?

tajila · 2019-01-25T15:58:06Z

sourcetools/j9constantpool/com/ibm/oti/VMCPTool/ConstantPoolStream.java

@@ -388,4 +388,4 @@ public void comment(String string) {
 			out.print("\t/* " + string + " */");
 		}
 	}
-}
+}


neeed to keep the newline

tajila · 2019-01-25T16:00:21Z

One minor comment rest LGTM

@dmitripivkine Please review the GC changes

@fjeremic Please review the JIT changes

fjeremic

JIT changes look ok. However I'll point you to [1] for thet styling of git commits. The body should be wrapped at 72 chracters so as to make reading git log consistent and easy. I would highly reccomend reading [2] on why this matters. You can also use GitHub keywords such as "Fixes" [3] to automatically close the corresponding issues once this PR is merged. I've made all the modifications to the commit message on your behalf:

Remove duplicate macros between j9nonbuilder.h and j9consts.h

Replace and remove macros in j9consts.h and replaced those macros
accordingly, each with their respective duplicate in j9nonbuilder.h.

Steps:

- Remove duplicate macros from j9consts.h
- Replace the removed macros with their respective duplicate for 
wherever they appeared in the repository (This includes the macros in
the DDR code).
- Move the Macros from j9nonbuilder.h into j9consts.h. This is because
j9consts.h has a lot less structs that are dependant on other header
files.
- Add `#include j9consts.h` where necessary
- Change the copyrights of the modified files

Fixes: #4198

Signed-off-by: Aidan Ha <qbha@edu.uwaterloo.ca>

[1] https://github.com/eclipse/openj9/blob/master/CONTRIBUTING.md#commit-guidelines
[2] https://chris.beams.io/posts/git-commit/
[3] https://help.github.com/articles/closing-issues-using-keywords/

dmitripivkine

There are a few files with copyright year change only (no real changes):

runtime/gc_glue_java/ConcurrentMarkingDelegate.hpp
runtime/gc_glue_java/MarkingDelegate.hpp
runtime/gc_glue_java/ObjectModelDelegate.hpp
runtime/vm/guardedstorage.c

These files should be excluded from change set.
When fixed this PR LGTM

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/gc/GCClassArrayClassSlotIterator.java

keithc-ca · 2019-02-07T16:27:16Z

Will I only need to sign off one commit in this case (sign off the latest commit)?

That's correct.

No, each commit needs a Signed-off-by: line.

keithc-ca

The changes look good to me. I'd like to see this rebased relative to a more recent commit (say no more than a day in the past).

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/gc/GCClassArrayClassSlotIterator.java

AidanHa · 2019-02-08T12:26:13Z

Sure, let me add the fixes and do a rebase to see if anything will change

Replace and remove macros in j9consts.h and replaced those macros accordingly, each with their respective duplicate in j9nonbuilder.h. Steps: - Remove duplicate macros from j9consts.h - Replace the removed macros with their respective duplicate for wherever they appeared in the repository (This includes the macros in the DDR code). - Move the Macros from j9nonbuilder.h into j9consts.h. This is because j9consts.h has a lot less structs that are dependant on other header files. - Add `#include j9consts.h` where necessary - Change the copyrights of the modified files Fixes: eclipse-openj9#4198 Signed-off-by: Aidan Ha <qbha@edu.uwaterloo.ca>

AidanHa · 2019-02-08T14:58:48Z

I've rebased this branch to Eclipse:master just over an hour ago, and ran a build on my local machine (which ended up being successful) @keithc-ca. I've made changes to address the comments above.

keithc-ca · 2019-02-08T15:33:46Z

Jenkins test sanity zlinux,win jdk8,jdk11

gacholio · 2019-02-10T18:58:14Z

Reverted this due to DDR compile failures in the internal builds.

keithc-ca · 2019-02-10T20:22:03Z

@AidanHa This might work in reverse: removing uses of J9JavaAccessFlags in favour of using constants in J9Consts.

AidanHa · 2019-02-11T13:24:53Z

I'm not too familiar with the DDR code but from my point of view, I noticed there are no macros in j9consts.h that map to the ddr namespace (in other words, no there’s no code like
/* @DDR_namespace: map_to_type=J9JavaAccessFlags */ in j9consts.h).

Perhaps when the DDR is being compiled, then it doesn't even look at j9consts.h? If that is the case, where can I go to fix this?

keithc-ca · 2019-02-11T14:49:41Z

If there were no internal IBM builds, your change would have been fine. If we're going to consider eliminating duplicate constants, we must use only those that also were available in the past so that old core files produced by IBM builds can be analyzed.

gacholio · 2019-02-11T15:52:31Z

While I agree that's good practice, there will be circumstances where the back compatibility pain will be too high. You can always use the DDR that shipped with the drop that produced the core to analyze it.

Remove macros in j9consts.h and j9nonbuilder.h, and replace those macros accordingly, each with their respective j9nonbuilder duplicate in j9javaaccessflags.h. Steps: - Remove duplicate macros from j9consts.h and j9nonbuilder.h -Replace those macros accordingly, each with their respective j9nonbuilder duplicate in j9javaaccessflags.h wherever they appeared in the repository (This includes the macros in the DDR code). -Modify runtime/ddr/vmddrstructs.properties to allow support for Java8 and older - Change the copyrights of the modified files Fixes: eclipse-openj9#4198 Fixes: eclipseGH-4287 Signed-off-by: Aidan Ha <qbha@edu.uwaterloo.ca>

AidanHa force-pushed the master branch from f88ad24 to 13db0b9 Compare January 14, 2019 18:43

fjeremic added the comp:vm label Jan 14, 2019

fjeremic reviewed Jan 14, 2019

View reviewed changes

AidanHa force-pushed the master branch 6 times, most recently from abf0327 to f99eaee Compare January 17, 2019 16:06

tajila reviewed Jan 22, 2019

View reviewed changes

AidanHa force-pushed the master branch 2 times, most recently from 40c03e8 to 67e8c7c Compare January 24, 2019 18:16

tajila reviewed Jan 25, 2019

View reviewed changes

tajila approved these changes Jan 25, 2019

View reviewed changes

AidanHa force-pushed the master branch from 67e8c7c to 999f589 Compare January 25, 2019 16:32

fjeremic changed the title ~~Removed Duplicate macros.~~ Remove duplicate macros between j9nonbuilder.h and j9consts.h Jan 28, 2019

fjeremic approved these changes Jan 28, 2019

View reviewed changes

dmitripivkine approved these changes Jan 31, 2019

View reviewed changes

dmitripivkine added comp:gc comp:jit labels Jan 31, 2019

AidanHa force-pushed the master branch from 999f589 to d025cc6 Compare February 1, 2019 12:25

pshipton requested a review from keithc-ca February 7, 2019 15:16

keithc-ca requested changes Feb 7, 2019

View reviewed changes

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/gc/GCClassArrayClassSlotIterator.java Show resolved Hide resolved

keithc-ca approved these changes Feb 7, 2019

View reviewed changes

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/gc/GCClassArrayClassSlotIterator.java Outdated Show resolved Hide resolved

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/gc/GCClassArrayClassSlotIterator.java Outdated Show resolved Hide resolved

AidanHa force-pushed the master branch from de75080 to 8b6343a Compare February 8, 2019 14:34

keithc-ca self-assigned this Feb 8, 2019

keithc-ca merged commit c436b1f into eclipse-openj9:master Feb 9, 2019

gacholio mentioned this pull request Feb 10, 2019

Revert "Remove duplicate macros between j9nonbuilder.h and j9consts.h" #4678

Merged

AidanHa mentioned this pull request Feb 19, 2019

Remove duplicate macros between j9nonbuilder.h and j9consts.h #4791

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove duplicate macros between j9nonbuilder.h and j9consts.h #4287

Remove duplicate macros between j9nonbuilder.h and j9consts.h #4287

AidanHa commented Jan 14, 2019

fjeremic left a comment

AidanHa commented Jan 14, 2019

fjeremic commented Jan 14, 2019

tajila Jan 22, 2019

tajila Jan 22, 2019

tajila Jan 22, 2019

tajila Jan 22, 2019 •

edited

Loading

tajila Jan 22, 2019

tajila Jan 22, 2019

tajila Jan 22, 2019

tajila Jan 22, 2019

tajila Jan 22, 2019

tajila Jan 22, 2019

AidanHa commented Jan 22, 2019 •

edited

Loading

tajila Jan 25, 2019

tajila commented Jan 25, 2019

fjeremic left a comment •

edited

Loading

dmitripivkine left a comment

keithc-ca commented Feb 7, 2019

keithc-ca left a comment

AidanHa commented Feb 8, 2019

AidanHa commented Feb 8, 2019

keithc-ca commented Feb 8, 2019

gacholio commented Feb 10, 2019

keithc-ca commented Feb 10, 2019

AidanHa commented Feb 11, 2019 •

edited

Loading

keithc-ca commented Feb 11, 2019

gacholio commented Feb 11, 2019

Remove duplicate macros between j9nonbuilder.h and j9consts.h #4287

Remove duplicate macros between j9nonbuilder.h and j9consts.h #4287

Conversation

AidanHa commented Jan 14, 2019

fjeremic left a comment

Choose a reason for hiding this comment

AidanHa commented Jan 14, 2019

fjeremic commented Jan 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tajila Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AidanHa commented Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

tajila commented Jan 25, 2019

fjeremic left a comment • edited Loading

Choose a reason for hiding this comment

dmitripivkine left a comment

Choose a reason for hiding this comment

keithc-ca commented Feb 7, 2019

keithc-ca left a comment

Choose a reason for hiding this comment

AidanHa commented Feb 8, 2019

AidanHa commented Feb 8, 2019

keithc-ca commented Feb 8, 2019

gacholio commented Feb 10, 2019

keithc-ca commented Feb 10, 2019

AidanHa commented Feb 11, 2019 • edited Loading

keithc-ca commented Feb 11, 2019

gacholio commented Feb 11, 2019

tajila Jan 22, 2019 •

edited

Loading

AidanHa commented Jan 22, 2019 •

edited

Loading

fjeremic left a comment •

edited

Loading

AidanHa commented Feb 11, 2019 •

edited

Loading