Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails #1682

mpirvu · 2018-04-13T13:05:46Z

Compiling OpenJ9 with a more recent compiler (e.g. gcc 5.x) can expose some
long existing bugs in two parts of the code:
(1) Allocating a TR_RelocationRecord and morphing it into a specialized
relocation record
(2) Copying an IlGeneratorMethodDetails object in
TR_MethodToBeCompiled::initialize()

In both cases the code constructs an object of a derived class on top of an
object of a base class using placement new. Of course, for this to have any
chance of success, the size of the two objects, base and derived, must be the
same and we make sure that indeed that's the case.
However, the code suffers from a subtle problem: the C++ compiler sees an
object of a base class being instantiated and it's free to devirtualize calls
off such an object, leading to the wrong code to be executed.

For clarification, let's use a generic example:

class B {
  public:
    B(param); // constructor
    virtual void foo();  
};
class D : public B {
  public:
    virtual void foo();
};
B::B(param) {
  if (param==...)
    new (this) D(); // constructing 'D' on top of existing object of type 'B'
}
---- main code ---
B b(param);
b.foo();  // A virtual call may call D::foo()
          // With devirtualization B::foo() is called always

The pattern used here is that of an object factory: based on some input we
want to create an object of a specific derived type, but return a pointer to
the base class so that it can be handled through virtual functions.
The difference from a traditional object factory is that we use placement
new to construct on top of an existing object (rather than allocating a brand
new object).
For the relocation record case mentioned above this approach has the advantage
of allocating a single TR_RelocationRecord on the stack and reusing that
storage for every record that the code needs to process.
For the ILGeneratorMethodDetails case this approach is somewhat forced by our
decision to embed such an object into TR_MethodToBeCompiled.

The simplest fix for the existing bugs is to use the object factory pattern
and allowing it to construct at a designated address, but always use the value
returned by the factory when calling virtual functions.
The example above could be modified along the following lines:

class B {
  ...
  // My object factory
  static B *create(B *storage, param) {
     if (param == ..) 
        return new (storage) D();
    ...
  }
};  

B storage; // this is where the new object is going to be created
B *b = B::create(storage, param); // create a specific object
b->foo();

mpirvu · 2018-04-13T13:07:51Z

@mstoodle Mark, could you please review the first commit related to relocation records? There will be another commit (under the same PR) for IlGeneratorMethodDetails. Thanks

mstoodle

It will work as is, but I think the code can be improved and only one of the recommended changes could be called "onerous" but hopefully still not too bad...

mstoodle · 2018-04-13T16:27:56Z

runtime/compiler/trj9/runtime/RelocationRecord.hpp

+
+      static TR_RelocationRecord *create(TR_RelocationRecord *storage, TR_RelocationRuntime *reloRuntime, TR_RelocationTarget *reloTarget, TR_RelocationRecordBinaryTemplate *recordPointer)
+         {
+         return decode(storage, reloRuntime, reloTarget, recordPointer);


Why not just fold decode() into create and keep create() out of the header? Nothing else calls decode, I don't think, so the extra call doesn't seem valuable to me.

mstoodle · 2018-04-13T16:39:40Z

runtime/compiler/trj9/runtime/RelocationRecord.cpp

         break;
      default:
         // TODO: error condition
         printf("Unexpected relo record: %d\n", reloType);fflush(stdout);
         TR_ASSERT(0, "Unexpected relocation record type found");
         exit(0);
      }
+      reloRecord->_reloRuntime = reloRuntime;


It's more code to change, but adding these fields to the constructors for all the relocation classes (which pass them into the base class constructor) to initialize would be a lot cleaner. Then you don't need to add the "setBinaryRecord()" function which isn't really something RelocationRecord should be allowing (especially as a public function).

mstoodle · 2018-04-13T16:45:31Z

runtime/compiler/trj9/runtime/RelocationRecord.hpp

@@ -216,6 +219,9 @@ class TR_RelocationRecord
      TR_RelocationRecordBinaryTemplate *_record;

      TR_RelocationRecordPrivateData _privateData;
+   private:
+      static TR_RelocationRecord * decode(TR_RelocationRecord *storage, TR_RelocationRuntime *reloRuntime, TR_RelocationTarget *reloTarget, TR_RelocationRecordBinaryTemplate *record);
+      static uint8_t getReloType(TR_RelocationTarget *reloTarget, TR_RelocationRecordBinaryTemplate *rec);


getReloType() is probably better implemented as a nonvirtual instance function on the TR_RelocationRecordBinaryTemplate struct. That's somewhat out of character for those structs, but I like that better than adding this function here.

getReloType() is used in a static function, so it has to be static as well (not an instance function). I could move it to TR_RelocationRecordBinaryTemplate, but still as a static function.

mpirvu · 2018-04-13T18:42:13Z

@mstoodle I have addressed the suggestions from the review.

When processing relocation records the JIT allocates a generic TR_RelocationRecord on the stack and then morphs this object into a specialized variety using placement new. The expectation is that any call off the newly created object is going to use the vtable to invoke the specific code associated with a specialized/derived relocation record. However, the C++ compiler is allowed to devirtualize calls and if that occurs, the JIT is going to invoke the implementation in the generic TR_RelocationRecord. The solution introduced by this commit is to use an object factory that builds a specific relocation record at a designated address and returns a pointer to that address. The newly created object must be accessed only through the pointer returned by the object factory. Signed-off-by: Marius Pirvu <[email protected]>

mstoodle · 2018-04-13T20:45:32Z

Looks good for this first change. Thanks, @mpirvu !

mstoodle · 2018-04-16T15:20:16Z

runtime/compiler/trj9/control/MethodToBeCompiled.cpp

@@ -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 = J9::IlGeneratorMethodDetails::create(_methodDetailsStorage, details);


This should not referenced J9::IlGeneratorMethodDetails directly. Only TR::IlGeneratorMethodDetails should be referenced outside of its class hierarchy.

You'll need to expose a static create() call in the TR:: version that directs down to the J9 static implementation.

I thought that for code within the trj9 folder it was ok to use J9::, or is it because IlGeneratorMethodDetails is extensible that we have to use TR::. But then I see instances in CompilationThread.cpp of J9::NewInstanceThunkDetails instead of TR::NewInstanceThunkDetails.

mstoodle · 2018-04-16T15:21:10Z

runtime/compiler/trj9/ilgen/IlGeneratorMethodDetails.hpp

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


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

mstoodle · 2018-04-16T15:22:12Z

runtime/compiler/trj9/ilgen/J9IlGeneratorMethodDetails.cpp

-   }
-
-}
-


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

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.

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

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.

mpirvu · 2018-04-16T18:26:06Z

Jenkins test all

mpirvu · 2018-04-17T01:49:27Z

Off the 6 builds that failed, 2 are connection issues, 2 are disk space issues and for two (JDK8 lnx 390/ppc) I see java.io.FileNotFoundException.

mpirvu · 2018-04-17T13:55:34Z

I verified manually on a Ubuntu 16.04 box (with gcc 5.4.0) that, before this fix, DLT methods were seen as OrdinaryMethods; after the fix they are correctly identified as MethodInProgress.

mpirvu · 2018-04-17T13:57:41Z

@mstoodle Mark, the code is ready to be merged, unless you would like more modifications. Thanks

mstoodle

still a couple things to clean up, please!

mstoodle · 2018-04-17T14:08:56Z

runtime/compiler/trj9/ilgen/J9IlGeneratorMethodDetails.cpp

   {
-   return self()->J9::IlGeneratorMethodDetailsConnector::operator=(other);


Just curious why this particular forwarding function has been put into the cpp file, which means little chance it will ever be inlined...?

I kept the old structure: wherever operator= was present I replaced it with the new clone function. You are right, it's better to move it in the hpp file. Done.

mstoodle · 2018-04-17T14:09:49Z

runtime/compiler/trj9/ilgen/J9IlGeneratorMethodDetails.hpp

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


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

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

When storing a particular variety of IlGeneratorMethodDetails into MethodToBeCompiled, the code uses operator = which transmutes the existing generic IlGeneratorMethodDetails (the base class) into a derived class similar to the parameter that is passed in, by using placement new. The expectation is that any call off the generic IlGeneratorMethodDetails object stored in MethodToBeCompiled is going to be a virtual call. However, the C++ compiler is allowed to devirtualize calls and if that occurs, the JIT is going to invoke the implementation in the generic IlGeneratorMethodDetails. The solution introduced by this commit is to use an object factory that builds a specific IlGeneratorMethodDetails at a designated address and returns a pointer to that address which is then stored in MethodToBeCompiled. Signed-off-by: Marius Pirvu <[email protected]>

mpirvu · 2018-04-23T23:05:14Z

I removed the clone() method from TR::IlGeneratorMethodDetails so that it can call directly the implementation from J9::IlGeneratorMethodDetails through inheritance.

mstoodle · 2018-04-24T18:44:14Z

jenkins test sanity

mstoodle · 2018-04-26T15:50:04Z

Great set of changes; thanks @mpirvu for fixing these long standing obstacles!

JamesKingdon · 2018-04-26T16:03:34Z

Time to warm up gcc 7 and see if we get any throughput improvements?

mpirvu · 2018-04-26T17:34:47Z

Throughput no, but maybe, maybe some startup time improvements from lower compilation time due to better devirtualization.

mpirvu added bug comp:jit labels Apr 13, 2018

mpirvu requested a review from mstoodle April 13, 2018 13:06

mpirvu changed the title ~~Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails~~ WIP: Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails Apr 13, 2018

dsouzai mentioned this pull request Apr 13, 2018

Relocatable Debug Counters #1589

Merged

fjeremic mentioned this pull request Apr 13, 2018

Support for compiling on gcc 7.3 #1684

Closed

mstoodle requested changes Apr 13, 2018

View reviewed changes

mpirvu force-pushed the relorecordfix branch 2 times, most recently from d920d4d to 50dc81e Compare April 13, 2018 18:31

mpirvu force-pushed the relorecordfix branch from 50dc81e to 2c9920a Compare April 13, 2018 19:45

mstoodle approved these changes Apr 13, 2018

View reviewed changes

mpirvu force-pushed the relorecordfix branch 2 times, most recently from 1216c84 to 2c9920a Compare April 16, 2018 14:33

mstoodle requested changes Apr 16, 2018

View reviewed changes

mpirvu force-pushed the relorecordfix branch from c3708ec to c715273 Compare April 16, 2018 17:44

mpirvu changed the title ~~WIP: Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails~~ Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails Apr 17, 2018

mstoodle requested changes Apr 17, 2018

View reviewed changes

mpirvu force-pushed the relorecordfix branch from c715273 to 8578cf3 Compare April 17, 2018 15:20

mpirvu force-pushed the relorecordfix branch from 8578cf3 to c394199 Compare April 23, 2018 22:08

mstoodle approved these changes Apr 24, 2018

View reviewed changes

mstoodle merged commit c9d35c2 into eclipse-openj9:master Apr 26, 2018

mpirvu deleted the relorecordfix branch December 13, 2018 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails #1682

Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails #1682

mpirvu commented Apr 13, 2018

mpirvu commented Apr 13, 2018

mstoodle left a comment

mstoodle Apr 13, 2018

mstoodle Apr 13, 2018

mstoodle Apr 13, 2018

mpirvu Apr 13, 2018

mpirvu commented Apr 13, 2018

mstoodle commented Apr 13, 2018

mstoodle Apr 16, 2018

dsouzai Apr 16, 2018 •

edited

Loading

mstoodle Apr 16, 2018

mstoodle Apr 16, 2018

mpirvu Apr 16, 2018

0xdaryl Apr 16, 2018

mpirvu Apr 16, 2018

mpirvu commented Apr 16, 2018

mpirvu commented Apr 17, 2018

mpirvu commented Apr 17, 2018

mpirvu commented Apr 17, 2018

mstoodle left a comment

mstoodle Apr 17, 2018 •

edited

Loading

mpirvu Apr 17, 2018

mstoodle Apr 17, 2018

mpirvu Apr 17, 2018

mpirvu commented Apr 23, 2018

mstoodle commented Apr 24, 2018

mstoodle commented Apr 26, 2018

JamesKingdon commented Apr 26, 2018 •

edited

Loading

mpirvu commented Apr 26, 2018

		{
		return self()->J9::IlGeneratorMethodDetailsConnector::operator=(other);

Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails #1682

Fix devirtualization issues with TR_RelocationRecord/IlGeneratorMethodDetails #1682

Conversation

mpirvu commented Apr 13, 2018

mpirvu commented Apr 13, 2018

mstoodle left a 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

mpirvu commented Apr 13, 2018

mstoodle commented Apr 13, 2018

Choose a reason for hiding this comment

dsouzai Apr 16, 2018 • 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

mpirvu commented Apr 16, 2018

mpirvu commented Apr 17, 2018

mpirvu commented Apr 17, 2018

mpirvu commented Apr 17, 2018

mstoodle left a comment

Choose a reason for hiding this comment

mstoodle Apr 17, 2018 • 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

mpirvu commented Apr 23, 2018

mstoodle commented Apr 24, 2018

mstoodle commented Apr 26, 2018

JamesKingdon commented Apr 26, 2018 • edited Loading

mpirvu commented Apr 26, 2018

dsouzai Apr 16, 2018 •

edited

Loading

mstoodle Apr 17, 2018 •

edited

Loading

JamesKingdon commented Apr 26, 2018 •

edited

Loading