Fix creation/initialization of TR::Options objects

[midronij]

This contribution includes the following changes:

• Add all non-static data members to TR::Options default constructor's initializer list
• Ensure _options array is initialized in all TR::Options constructors
• Ensure constructors are used to create new TR::Options objects (rather than memset)

Additionally, this contribution fixes #6556

[midronij]

@gita-omr

[0xdaryl]

Jenkins build all

[0xdaryl]

See the CI failure. This PR is intended to address that failure, no?

[babsingh]

https://ci.eclipse.org/omr/job/PullRequest-linux_ppc-64_le_gcc/3673/

• compunittest AbsVPValueTest segfaults #6556 is fixed since AbsVPValueTest passes.
• compunittest Special/PPCDirectEncodingTest Segfaults #6571 is seen where Special/PPCDirectEncodingTest fails.

[midronij]

@0xdaryl The failure that occurs here is a separate issue that's still being investigated. But the issue linked in the description is solved, since AbsVPValueTest passes

[gita-omr]

Would not it be better to initialize just _options[] array instead of the whole TR::Options object which might include internal C++ metadata like vft pointer?

[midronij]

@gita-omr I initialized it the same way the options are initialized in processOptionsJIT() (_jitCmdLineOptions is a TR::Options pointer): https://github.com/eclipse/omr/blob/a57ad601d6664d97958c59884c4d34a0ce2f960a/compiler/control/OMROptions.cpp#L2410

But if it would be more appropriate in this particular situation to only initialize the _options[] array, I can do that instead

[gita-omr]

@gita-omr I initialized it the same way the options are initialized in processOptionsJIT() (_jitCmdLineOptions is a TR::Options pointer):

https://github.com/eclipse/omr/blob/a57ad601d6664d97958c59884c4d34a0ce2f960a/compiler/control/OMROptions.cpp#L2410

But if it would be more appropriate in this particular situation to only initialize the _options[] array, I can do that instead

Right, but that code does not seem safe to me... On another hand I agree that it should be consistent.

@0xdaryl @mpirvu what do you think about initializing just the _options[] array in both places?

[midronij]

.

[mpirvu]

is it ok for the linux_ppc-64_le to fail?

[mpirvu]

LGTM

[midronij]

is it ok for the linux_ppc-64_le to fail?

Yup, that's being caused by a separate separate failure that's under investigation elsewhere

[0xdaryl]

I share @gita-omr's concerns about safety and correctness. I think we should do the correct and safe C++ thing here and either use a member initialization list or explicitly initialize them in the constructor.

[midronij]

@0xdaryl I've modified the constructor so that only the _options[] array (which holds the flag that was causing the problem due to being uninitialized) is being explicitly initialized to 0, rather than the entire TR::Options object. When you have a chance, could you review, and if everything is ok, please merge?

[mpirvu]

I've modified the constructor so that only the _options[] array

While I am not opposed to this, I would like to say that this solutions makes the code incrementally better, and allows a particular test to pass. I am guessing that the exact same issue can happen with one of the many other fields of TR_Options.

[mpirvu]

Before this change the code was initializing the entire Options object to 0 (thus, all fields were set to 0).
After this change, only the _options array is initialized to 0 and the other fields may receive random values.
We need to keep this line or write a full constructor that initializes all fields to 0, not just the _options array.

[midronij]

Yup, I can confirm that it's not enough to just initialize the _options array or even to use the default constructor. If we don't want to use memset we'll have to create a new constructor as you say.

The default constructor is actually used in the other branch of this if statement:
https://github.com/eclipse/omr/blob/a57ad601d6664d97958c59884c4d34a0ce2f960a/compiler/control/OMROptions.cpp#L2405
and it seems to work perfectly fine, but given how finnicky this bug was, I'm not sure if it's intentional or just a fluke.

[mpirvu]

This memset is also not needed, because the contructor does it now inside.

[midronij]

Do we not need to reset the _options array to all 0's? I thought that was the original purpose of using memset there in the first place. Otherwise there would have been no need for the if(_jitCmdLineOptions) branch at all

[mpirvu]

The constructor with no arguments TR::Options() initializes the options array:
memset(_options, 0, sizeof(_options));.
Because of this, when we execute: _jitCmdLineOptions = new (PERSISTENT_NEW) TR::Options(); all fields are correctly initialized to 0, including the _options array.

[gita-omr]

I am also not sure about this code. If processOptionsJIT is called only once , we don't need that memset. If it's called more than once and we would like to reinitialize the whole TR::Options object why do we only reinitialize _options[] ?

[midronij]

To address the possibility of processOptionsJIT() being called more than once and perserve the original intended functionality of this part of the code, I have created an init() function for TR::Options that can be used both by the default constructor and in procesOptionsJIT() when _jitCmdLineOptions has already been allocated but needs to be reinitialized.

It ends up looking like this:

 if (!_jitCmdLineOptions)
      {
      _jitCmdLineOptions = new (PERSISTENT_NEW) TR::Options();
      _cmdLineOptions = _jitCmdLineOptions;
      }
 else
      _jitCmdLineOptions->init();

[mpirvu]

I am wondering whether c++11 class memory initialization is supported on all our platforms. If it is, we could simplify the initialization code like this:

         TR::OptionSet  *_optionSets {NULL};
          char *         _startOptions {NULL};
          char *         _envOptions {NULL};
          uint32_t      _options[TR_OWM+1] {0};
          ...

[midronij]

@mpirvu unfortunately C++11 class memory initialization works for everything except AIX (for java 8 specifically) or zOS. So unless there's a workaround for that, it looks like we'll have to stick with an initialization list for the default constructor

[midronij]

@mpirvu @0xdaryl I've corrected my changes to use an initialization list in the default TR::Options constructor instead of memset. When you have a chance, could you please review and (if everything looks ok) merge?

[dsouzai]

Sorry, I don't mean to cause so much churn. However, thinking more on this, I don't think doing self()->init() is safe.

Normally calling a member method using self()-> is the way to go. However, this is kind of special in that init() is being called from the constructor. If a downstream project was to extend OMR::Options and define its own init(), we'd technically end up calling the overriding init() before that subtype's constructor. I don't know what the consequence of something like that is, but we probably shouldn't allow that.

My recommendation is, In OMROptions.hpp, define the empty constructor as:

Options() { OMR::Options::init(); }

This way there's no ambiguity, and we're not initializing the part of the object that hasn't technically been constructed yet. You can see an example of this in OMR::Compilation:
https://github.com/eclipse/omr/blob/0db5d7c559b8b47754f1264f73f54c7ca8c7b374/compiler/compile/OMRCompilation.hpp#L857

[midronij]

@dsouzai no worries, it's a valid consideration that I haven't thought of. Just made the change now

[dsouzai]

jenkins build all

[AdamBrousseau]

jenkins build zos

[gita-omr]

We could replace memset with init() in processOptionsAOT as well.

[gita-omr]

Please also make sure that new members have not been added to TR::Options since this code was developed.

[gita-omr]

Jenkins build all

[dsouzai]

riscv failure is a timeout, similar to #6665