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

Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails #1682

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails #1682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c9920a
Fix the devirtualization issue with TR_RelocationRecord
mpirvu Apr 13, 2018
c394199
Fix devirtualization issues with IlGeneratorMethodDetails
mpirvu Apr 16, 2018
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
4 changes: 2 additions & 2 deletions runtime/compiler/trj9/control/MethodToBeCompiled.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2016 IBM Corp. and others
* Copyright (c) 2000, 2018 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -50,7 +50,7 @@ TR_MethodToBeCompiled *TR_MethodToBeCompiled::allocate(J9JITConfig *jitConfig)

void TR_MethodToBeCompiled::initialize(TR::IlGeneratorMethodDetails & details, void *oldStartPC, CompilationPriority p, TR_OptimizationPlan *optimizationPlan)
{
_methodDetails = details;
_methodDetails = TR::IlGeneratorMethodDetails::clone(_methodDetailsStorage, details);
_optimizationPlan = optimizationPlan;
_next = NULL;
_oldStartPC = oldStartPC;
Expand Down
7 changes: 4 additions & 3 deletions runtime/compiler/trj9/control/MethodToBeCompiled.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2016 IBM Corp. and others
* Copyright (c) 2000, 2018 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -52,7 +52,7 @@ struct TR_MethodToBeCompiled
void initialize(TR::IlGeneratorMethodDetails & details, void *oldStartPC, CompilationPriority p, TR_OptimizationPlan *optimizationPlan);

TR::Monitor *getMonitor() { return _monitor; }
TR::IlGeneratorMethodDetails & getMethodDetails(){ return _methodDetails; }
TR::IlGeneratorMethodDetails & getMethodDetails(){ return *_methodDetails; }
bool isDLTCompile() { return getMethodDetails().isMethodInProgress(); }
bool isCompiled();
bool isJNINative();
Expand All @@ -62,7 +62,8 @@ struct TR_MethodToBeCompiled


TR_MethodToBeCompiled *_next;
TR::IlGeneratorMethodDetails _methodDetails;
TR::IlGeneratorMethodDetails _methodDetailsStorage;
TR::IlGeneratorMethodDetails *_methodDetails;
void *_oldStartPC;
void *_newStartPC;
TR::Monitor *_monitor;
Expand Down
4 changes: 2 additions & 2 deletions runtime/compiler/trj9/ilgen/IlGeneratorMethodDetails.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2016 IBM Corp. and others
* Copyright (c) 2000, 2018 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -50,8 +50,8 @@ class OMR_EXTENSIBLE IlGeneratorMethodDetails : public J9::IlGeneratorMethodDeta
IlGeneratorMethodDetails(const TR::IlGeneratorMethodDetails & other) :
J9::IlGeneratorMethodDetailsConnector(other) {}

IlGeneratorMethodDetails & operator=(const TR::IlGeneratorMethodDetails & other);
Copy link
Contributor

Choose a reason for hiding this comment

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

if you want that create function, you'll need to declare it here

};

}


Expand Down
36 changes: 12 additions & 24 deletions runtime/compiler/trj9/ilgen/J9IlGeneratorMethodDetails.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2016 IBM Corp. and others
* Copyright (c) 2000, 2018 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -35,47 +35,35 @@
#include "ilgen/J9ByteCodeIlGenerator.hpp"
#include "trj9/env/VMJ9.h"

namespace TR
{

IlGeneratorMethodDetails & IlGeneratorMethodDetails::operator=(const TR::IlGeneratorMethodDetails & other)
{
return self()->J9::IlGeneratorMethodDetailsConnector::operator=(other);
}

}

Copy link
Contributor

Choose a reason for hiding this comment

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

...and you'll need to implement create() here to call J9::IlGeneratorMethodDetails::create()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly what I did, but I hit a strange compilation error in trj9/env/j9method.cpp when using the other create function:
TR::IlGeneratorMethodDetails & details = TR::IlGeneratorMethodDetails::create(storage, this);
It's like the compiler cannot distinguish between
inline static TR::IlGeneratorMethodDetails & create(TR::IlGeneratorMethodDetails & target, TR_ResolvedMethod *method);
and
static IlGeneratorMethodDetails *create(TR::IlGeneratorMethodDetails &storage, const TR::IlGeneratorMethodDetails & other);
I thought that TR_ResolvedMethod *method was considered different than TR::IlGeneratorMethodDetails & other
Changing the name of the second create function to something else solves the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you need a using J9::IlGeneratorMethodDetailsConnector::create; in your J9IlGeneratorMethodDetails.hpp. We've seen this before with extensible classes where we're directing the compiler to search for a method from the leaves of the hierarchy upward, and if there are two or more methods with the same name but different signatures this instructs the compiler to keep searching if it encounters a signature that doesn't match. I don't recall the name of this C++ phenomenon. See compiler/x/codegen/OMRCodeGenerator.hpp where we use it for canNullChkBeImplicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I am still going to change the name of function I added to 'clone' because it better reflects what it does: it takes an object and it allocates an identical object at the specified address.



namespace J9
{

TR::IlGeneratorMethodDetails &
IlGeneratorMethodDetails::operator=(const TR::IlGeneratorMethodDetails & other)
TR::IlGeneratorMethodDetails *
IlGeneratorMethodDetails::clone(TR::IlGeneratorMethodDetails &storage, const TR::IlGeneratorMethodDetails & other)
{
// The purpose of this function is to transmute the receiver object (*this) to make it identical to "other"
// Placement new is used to get the vft updated, and constructors on each of the classes identified here
// take care of copying the appropriate data fields. The if nest below covers every concrete subclass of
// IlGeneratorMethodDetails. If other is not one of these classes, then it will assert.
// The if nest below covers every concrete subclass of IlGeneratorMethodDetails.
// If other is not one of these classes, then it will assert.

if (other.isOrdinaryMethod())
return * new (self()) TR::IlGeneratorMethodDetails(static_cast<const TR::IlGeneratorMethodDetails &>(other));
return new (&storage) TR::IlGeneratorMethodDetails(static_cast<const TR::IlGeneratorMethodDetails &>(other));
else if (other.isDumpMethod())
return * new (self()) DumpMethodDetails(static_cast<const DumpMethodDetails &>(other));
return new (&storage) DumpMethodDetails(static_cast<const DumpMethodDetails &>(other));
else if (other.isNewInstanceThunk())
return * new (self()) NewInstanceThunkDetails(static_cast<const NewInstanceThunkDetails &>(other));
return new (&storage) NewInstanceThunkDetails(static_cast<const NewInstanceThunkDetails &>(other));
else if (other.isMethodInProgress())
return * new (self()) MethodInProgressDetails(static_cast<const MethodInProgressDetails &>(other));
return new (&storage) MethodInProgressDetails(static_cast<const MethodInProgressDetails &>(other));
else if (other.isMethodHandleThunk())
{
if (static_cast<const MethodHandleThunkDetails &>(other).isShareable())
return * new (self()) ShareableInvokeExactThunkDetails(static_cast<const ShareableInvokeExactThunkDetails &>(other));
return new (&storage) ShareableInvokeExactThunkDetails(static_cast<const ShareableInvokeExactThunkDetails &>(other));
else if (static_cast<const MethodHandleThunkDetails &>(other).isCustom())
return * new (self()) CustomInvokeExactThunkDetails(static_cast<const CustomInvokeExactThunkDetails &>(other));
return new (&storage) CustomInvokeExactThunkDetails(static_cast<const CustomInvokeExactThunkDetails &>(other));
}

TR_ASSERT(0, "Unexpected IlGeneratorMethodDetails object\n");
return *(self());
return NULL; // error case
}


Expand Down
6 changes: 3 additions & 3 deletions runtime/compiler/trj9/ilgen/J9IlGeneratorMethodDetails.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2017 IBM Corp. and others
* Copyright (c) 2000, 2018 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -74,10 +74,10 @@ class OMR_EXTENSIBLE IlGeneratorMethodDetails : public OMR::IlGeneratorMethodDet

IlGeneratorMethodDetails(const TR::IlGeneratorMethodDetails & other);

TR::IlGeneratorMethodDetails & operator=(const TR::IlGeneratorMethodDetails & other);

static TR::IlGeneratorMethodDetails & create(TR::IlGeneratorMethodDetails & target, TR_ResolvedMethod *method);
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no longer any create() implementation, is there? this declaration should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the old create() function that is used in two places in the code. Note that the second parameter is a TR_ResolvedMethod.


static TR::IlGeneratorMethodDetails * clone(TR::IlGeneratorMethodDetails & storage, const TR::IlGeneratorMethodDetails & source);

virtual const char * name() const { return "OrdinaryMethod"; }

virtual bool isOrdinaryMethod() const { return true; }
Expand Down
Loading