-
Notifications
You must be signed in to change notification settings - Fork 736
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
cmdLineTester_callsitedbgddrext_openj9 test failing with mixed references build #11469
Comments
This is not limited to mixed references build. adoptium/run-aqa#50 |
If you take the zlinux mixed build, create a core file, and open it with jdmpview using the
|
@sophia-guo you should open a new issue for the aqa failure, it's a different problem. This issue is specific to the mixed builds. |
Still working on identifying the root problem here. I added some prints to check the With a core file generated by a mixed refs build on a BE platform, the output is false for all configurations/settings:
However, the expectation is to see:
It seems that something might be going wrong in the initialization of GCExtensions or while the GCExtensions configurations are getting set. However, printing out the GCExtensions settings while the VM is running normally (not in jdmpview) yields the correct settings ( I'm looking through the DDR/jdmpview code and trying to run some dump commands on the core file for more insights, but I'm not very familiar with jdmpview and welcome any additional pointers/suggestions! |
Doesn't seem like it would be the same issue, but FYI #11639 How do you print the J9BuildFlags? Try dumping the raw memory instead of using DDR to print the values. |
If you can find the blob in the core file, you could extract it with |
There's also the |
#3837 seems relevant to this issue. I was able to print the flags in I'm not able to use |
Buildflags output
|
The important ones in this case:
|
I think the real problem is here:
We look at the runtime state of So the problem is not with DDR, we need to determine if the GC configuration code in OMR is being run properly.
|
I ran the VM under gdb:
|
Okay, I think DDR is wrong now: Here is the DDR output:
Here are the raw bytes annotated:
|
With Sharon's help we discovered that the offset shown in gdb is: _isSegregatedHeap // offset 0x5860 This differs from the offsets DDR is reporting |
Sharon and I have discovered that DDR has an extra field. |
|
blob file:
|
I suspect this is a consequence of not compiling all source files with the same macro definitions. If |
When is the DWARF description of
|
is true, then the unexpected field will be found and have consequences for the offsets of following fields. |
These changes to add a dummy function work to fix this issue: master...sharon-wang:crdummy and eclipse-omr/omr@master...sharon-wang:crdummy. The reason why
is because the compiler complained about an unused field However, adding Does this seem like a reasonable fix? It is a bit of a hack... FYI @gacholio. |
If we decide that the 'dummy' functions are a reasonable solution, they shouldn't need to be defined (anywhere). I don't see how a compiler could reasonably complain that a field is unused if there are any non-inline functions (or friends) declared in a class (it might be used by the implementation of that non-inline member (or accessed by a friend class)). |
Maybe it's just the compiler on my mac, where I get
if I just remove However, it seems to work on other machines without the unused private field error. I can try testing on a few different platforms to see if this is specific to mac. |
Removing The 'dummy' function could be ifdeffed on |
If it's only the one compiler, a better solution might be to disable that warning. |
The issue that will remain is inconsistent type definitions that will break DDR; see #11469 (comment). |
If the override ifdef is removed, doesn't that take care of the inconsistency? DDR and the real compile will be the same. |
Yes, if we remove |
I added an extra field (for easy identification) next to |
So, compiling that file with the override would also fix this? That would be a better overall solution. |
Yes, I think that would fix it. I'd like to see a change that would catch any other mis-matches: Ideas on how to do that, anyone? |
A second file that is missing that macro definition is |
So we'll keep existing changes as is, but we'll split
I think it'd work if we swapped
We'd have to update
I think this would fix things without having to split the two libraries mentioned. |
I don't understand how that fixes anything. It doesn't matter what preprocessor conditions we use, but we do need to have a consistent set of fields in all structures (at consistent offsets). For
|
Using
Yes, that is correct. So all the |
Can't the field just always be included? |
We could, but clang on mac complains about the field being unused, since it doesn't get used in mixed refs builds. So we'd need to add a dummy function that "uses" it, or we can disable the |
I'm leaning back towards doing this this with the dummy function or compiler warning disable for OSX. This means the field would always be there in the mixed builds no matter the override/splitting. |
The error on mac talks about private fields - what if we just make all of the |
I agree the field should always be present. I also think it makes sense to have an accessor function (that would be used in code that isn't split and needs to query that state). |
If a non-split class uses the affected header, it will get the existing accessor, which will query the field - there is no need for another function that no one will ever call. |
Fair enough. Then we just need to make the existence of such fields consistent and suppress the warning from xcode. |
I'm hoping that making the fields public will avoid the warning. |
Is there a preference to make the fields |
I suppose protected is technically better, though I'm sure no one is actually worried about the visibility of the field. Please give it a try with protected. |
Switching the field to |
To be clear, the field is now protected, and the !defined(override) has been removed from everywhere? |
Yes, the field is protected in all the header files that declare it, and |
Opened: #11691 and eclipse-omr/omr#5758 with the fix. Thank you everyone for your help investigating this! |
Failure link: https://ci.eclipse.org/openj9/job/Test_openjdk11_j9_sanity.functional_s390x_linux_mixed_cm_Nightly/2/tapResults/
functional
sanity.functional
ppc64_aix_mixed_cm
--> SPEC renamed toppc64_aix_mixed
s390x_linux_mixed_cm
--> SPEC renamed tos390x_linux_mixed
cmdLineTester_callsitedbgddrext_openj9_0
!printallcallsites
,!findallcallsites
Version info (only failing with mixedrefs CMake build)
Discussion: #9284 and https://openj9.slack.com/archives/C8312LCV9/p1607532450200200
Possibly related: adoptium/run-aqa#50
Mixed Refs PRs:
Optional info
To generate a "bad core file":
--with-mixedrefs --with-cmake
on BE machine, such ass390x_linux
orppc64_aix
java -Xdump:system:events=vmstop -version
Failure output
The text was updated successfully, but these errors were encountered: