JDK-8160404 : RelocationHolder constructors have bugs
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 9,10,11,17,20
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2016-06-27
  • Updated: 2022-12-22
  • Resolved: 2022-12-16
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 21
21 b03Fixed
Related Reports
Duplicate :  
JDK-8160354 - uninitialized value warning and VM crash are occurred with GCC 6
Relates :  
JDK-8160310 - HotSpot cannot be built with GCC 6
Relates :  
JDK-8187676 - Disable uninitialized warnings for two files until proper fix available
Relates :  
JDK-8298912 - Make RelocationHolder::reloc const-correct
Relates :  
JDK-8299022 - Linux ppc64le and s390x build issues after JDK-8160404
Relates :  
JDK-8297830 - aarch64: Make Address a discriminated union internally
Relates :  
JDK-8298913 - Add override qualifiers to Relocation classes
Relates :  
JDK-8160354 - uninitialized value warning and VM crash are occurred with GCC 6
Relates :  
JDK-8253311 - Cleanup relocInfo constructors
Description
RelocationHolder has a _relocbuf member, which is really just storage for a Relocation object.  The constructors for RelocationHolder are both problematic.  The no-arg constructor is

RelocationHolder::RelocationHolder() {
  new(*this) Relocation();
}

This is all very contorted and fragile. I wonder why RelocationHolder doesn't just use placement new to (default) construct the Relocation object. e.g.

new (_relocbuf) Relocation();

The other constructor is

RelocationHolder::RelocationHolder(Relocation* r) {
  // wordwise copy from r (ok if it copies garbage after r)
  for (int i = 0; i < _relocbuf_size; i++) {
    _relocbuf[i] = ((void**)r)[i];
  }
}

and that comment is just wrong, since the actual object could have been allocated close to the end of accessible memory, with a read beyond its real end potentially resulting in some kind of memory fault.
Comments
Changeset: bfa921ae Author: Kim Barrett <kbarrett@openjdk.org> Date: 2022-12-16 20:47:40 +0000 URL: https://git.openjdk.org/jdk/commit/bfa921ae6ce068c53dfa708d6d3d2cddbad5fc33
16-12-2022
A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/11618 Date: 2022-12-09 22:00:59 +0000
09-12-2022
I tried removing the warning suppressions from JDK-8187676, and there is no problem building with gcc10.3 on x64, for all of release, fastdebug, and slowdebug configurations. I think the warnings were (and still would be) correct, so don't know why we're not getting them anymore. I don't know what might have changed in the meantime.
18-02-2022
[~jrose], looking at the SCCS history, it looks like you may have added RelocationHolder. The commit comment says "+NewRelocs", which seems to be an old flag that enabled new relocation code and classes. Do you happen to remember the reason for RelocationHolder? Was it all about supporting stack allocation?
15-02-2022
I was assuming the reason for RelocationHolder was to avoid using the C++ heap and to support the few cases that are using polymorphism. I would be surprised if the object lifetimes were complicated enough to require reference counts. Relocation objects are temporary objects that we translate to/from the compressed persistent form. I think we should be able to remove RelocationHolder completely by rewriting problematic code. For example, 1822 RelocationHolder rspec = _optimized_virtual ? opt_virtual_call_Relocation::spec(method_index) 1823 : static_call_Relocation::spec(method_index); 1824 emit_d32_reloc(cbuf, [...], rspec, [...]); could be changed to: if (_optimized_virtual) { opt_virtual_call_Relocation reloc(method_index); emit_d32_reloc(cbuf, [...], reloc, [...]); } else { static_call_Relocation reloc(method_index); emit_d32_reloc(cbuf, [...], reloc, [...]); } ... if we really wanted to preserve stack allocation. It should be even simpler if we allocated from a ResoureArea instead of the stack. To minimize the amount of changes, we could keep RelocationHolder around as a handle for the Relocation*.
14-02-2022
I assume the problem with that is keeping track of the lifetime of the Relocation object, so we know when to delete it. I don't know the code, so don't know how hard that is, but I've always assumed it's hard enough to justify the holder mechanism. The holder passes it around by copy. Another option would be refcounting the Relocation objects. Indeed, RelocationHolder could probably be reimplemented as a reference counting handle. Presumably someone thought the cost of reference counting would outweigh the copying cost by enough to matter. Though getting the holder to be conforming C++ is not so easy either, and the existing code is a long way off.
13-02-2022
This may be a dumb question, but why do we need RelocationHolder at all? Can't we just pass the Relocation around as a const Relocation *?
25-01-2018
Notes - 01. Attached '8160404_03.diff' with last updates incorporating following review comments from Kim Barrett on earlier '8160404_02.diff'. (adding notes, extracts from emails for record) ================================= > Changing rspec.reloc()->type() to rspec.type(). > There are a few more occurences of that pattern in other files that should be updated. > x86_32.ad / macroAssembler_s390.inlin.hpp / macroAssembler_aarch64.cpp -- Yes, done. ================================= > src/share/vm/code/relocInfo.hpp > 837 new((RelocationHolder &) _relocbuf) Relocation(); > I think the cast to RelocationHolder& is wrong. > We're really doing placement new of an as yet uninitialized _relocbuf here, and should just treat _relocbuf as memory. > The "new" being referenced here should be the global placement new function, not Relocation::operator new, so it probably needs to be explicitly qualified, > e.g. I think line 837 should be > ::new (_relocbuf) Relocation(); > or possibly better is to have Relocation also provide an overload for operator placement new, like > void* operator new(size_t size, void* p) throw() { return p; } > I see that Hotspot contains a number of qualified ::new calls in order to dodge the operator new provided by our various allocation controlling base classes > and get to the global placement new. > So using ::new on line 837 would be culturally consistent -- Added overloaded operator placement new in Relocation and used the same in no-arg constructor, RelocationHolder(). ================================= > src/share/vm/code/relocInfo.hpp > The various clone_into implementatoins are doing raw memory copies; > they shouldn't be. Instead, they should be doing placement copies. > Also, I think clone_into should take a void* rather than an array of void*. > So, e.g. for DataRelocation, I think it should be something like > void clone_into(void* ptr, size_t size) const { > assert(sizeof(DataRelocation) <= size, "..."); > ::new (ptr) DataRelocation(*this); > } > [Note again here the use of explicitly qualified global new.] > -- Yes changes the clone_into implementations also to use the added overloaded operator placement new of Relocation. ================================= > src/share/vm/asm/codeBuffer.cpp > 393 end->initialize(this, const_cast<Relocation*>(reloc)); > I really dislike this const_cast, but on a quick look it appears removing it and making things const-correct might propogate quite far. > I'd be interested in finding out how bad it gets though. If it really is excessive, then maybe a followup cleanup? -- Tried propagating the constness and please check the required usage of mutable now. 02. Though confirmed no build issues locally with attached '8160404_03.diff' changeset with existing gcc versions, But Erik Helin helped confirm following warnings still present, with this 8160404_03.diff patch, when building hotspot with gcc7 - >> tested to compile the patch with "g++ (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)" on Fedora 26 and unfortunately we still get the following warnings: [edvbld@edvbld-personal hs]$ make hotspot Building target 'hotspot' in configuration 'linux-x86_64-normal-server-release' /home/edvbld/code/openjdk/10/hs/src/hotspot/cpu/x86/assembler_x86.cpp: In static member function ‘static Address Address::make_raw(int, int, int, int, relocInfo::relocType)’: /home/edvbld/code/openjdk/10/hs/src/hotspot/cpu/x86/assembler_x86.cpp:199:12: error: ‘*((void*)& rspec +32)’ may be used uninitialized in this function [-Werror=maybe-uninitialized] return madr; ^~~~ /home/edvbld/code/openjdk/10/hs/src/hotspot/cpu/x86/assembler_x86.cpp:199:12: error: ‘*((void*)& rspec +24)’ may be used uninitialized in this function [-Werror=maybe-uninitialized] /home/edvbld/code/openjdk/10/hs/src/hotspot/cpu/x86/assembler_x86.cpp:199:12: error: ‘*((void*)& rspec +16)’ may be used uninitialized in this function [-Werror=maybe-uninitialized] cc1plus: all warnings being treated as errors 03. Latest comments from Kim for the record - (not able to make progress on this yet) --------- " Looks like we don't have an assignment operator for RelocationHolder, but need one. Which suggests we also need a copy constructor.. Something like this, perhaps? RelocationHolder& RelocationHolder::operator=(const RelocationHolder& x) { if (this != &x) { reloc()->~Relocation(); x.clone_into(_relocbuf, sizeof(_relocbuf)); } return *this; } RelocationHolder::RelocationHolder(const RelocationHolder& x) { x.reloc()->clone_into(_relocbuf, sizeof(_relocbuf)); } Also, I think Relocation should not have public (implicit) copy constructor or assignment operator. I also just noticed that we’re missing a virtual destructor for Relocator, which we definitely need. But this is feeling yet more contorted. It seems like there ought to be a better way, that is still correct. Unfortunately, I don't have time to think about it right now, but I feel like much of the use of operator new in these classes is ill-conceived. Relocator/Relocation/ I’ve just been assuming its destructor was virtual, since it always should have been. I really wish we could turn on the appropriate gcc warning flag (-Wnon-virtual-dtor). " ----------- 04. Please note for now plan to defer this JDK-8160404 task to jdk11. Understood the related JDK-8187676 is also blocked because of this JDK-8160404. Since 8160404 is taking more time, if it is okay, as Erik already suggested, can we please go ahead and disable the gcc warnings at the moment as part of 8187676 fix and re-enable latest as part of 8160404 patch. ref : http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-September/027138.html
28-11-2017
related - http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-November/027719.html
28-11-2017
Found no issues with testing - jprt (-testset hotspot) and RBT (--job hs-precheckin-comp) for all above suggested, attached changes.
12-09-2017
wrt to above comments - https://bugs.openjdk.java.net/browse/JDK-8160404?focusedCommentId=14113088&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14113088 trying out fix proposal changes as per attached - 8160404_02.diff Notes - [1]. 'changes in operator Relocation::new(), RelocationHolder::RelocationHolder()' --- [relocInfo.hpp] ========================== - void* operator new(size_t size, const RelocationHolder& holder) throw() { + void* operator new(size_t size, RelocationHolder& holder) throw() { if (size > sizeof(holder._relocbuf)) guarantee_size(); assert((void* const *)holder.reloc() == &holder._relocbuf[0], "ptrs must agree"); - return holder.reloc(); + holder.reloc()->~Relocation(); + return holder._relocbuf; } .................... inline RelocationHolder::RelocationHolder() { // initialize the vtbl, just to keep things type-safe - new(*this) Relocation(); + STATIC_ASSERT(sizeof(Relocation) <= sizeof(_relocbuf)); + new((RelocationHolder &) _relocbuf) Relocation(); } ========================== [2]. 'RelocationHolder::reloc() related' trying changes - --- [relocInfo.hpp] ========================== - Relocation* reloc() const { return (Relocation*) &_relocbuf[0]; } + Relocation* reloc() { return (Relocation*)_relocbuf; } + const Relocation* reloc() const { return (const Relocation*)_relocbuf; } ========================== and related following type changes required - .e.g.: --- [relocInfo.hpp] ========================== public: - virtual relocInfo::relocType type() { return relocInfo::none; } + virtual relocInfo::relocType type() const { return relocInfo::none; } // is it a call instruction? - virtual bool is_call() { return false; } + virtual bool is_call() const { return false; } // is it a data movement instruction? - virtual bool is_data() { return false; } + virtual bool is_data() const { return false; } ............... class DataRelocation : public Relocation { public: - bool is_data() { return true; } + bool is_data() const { return true; } ............... class CallRelocation : public Relocation { public: - bool is_call() { return true; } + bool is_call() const { return true; } ............... class oop_Relocation : public DataRelocation { - relocInfo::relocType type() { return relocInfo::oop_type; } + relocInfo::relocType type() const { return relocInfo::oop_type; } ............... ========================== --- [assembler_x86.cpp] ========================== - Relocation* r = rspec.reloc(); + const Relocation* r = rspec.reloc(); ........... - emit_data(disp, rspec.reloc(), call32_operand); + emit_data(disp, rspec, call32_operand); ========================== --- [x86_64.ad] ========================== - if (rspec.reloc()->type() == relocInfo::oop_type && + if (rspec.type() == relocInfo::oop_type && ............. ========================== --- [codeBuffer.cpp] ========================== void CodeSection::relocate(address at, RelocationHolder const& spec, int format) { .......... - Relocation* reloc = spec.reloc(); + const Relocation* reloc = spec.reloc(); .......... - end->initialize(this, reloc); + end->initialize(this, const_cast<Relocation*>(reloc)); ========================== [3]. 'fixing RelocationHolder(Relocation*)' [3.1]. Adding clone_into() .e.g.: --- [relocInfo.hpp] ========================== + // used for creating RelocationHolder from Relocation object + virtual void clone_into(void* ptr[], size_t size) const; + ....... ....... class DataRelocation : public Relocation { ........ + void clone_into(void* ptr[], size_t size) const { + guarantee(sizeof(DataRelocation) <= size, "Make _relocbuf bigger!"); + for (unsigned int i = 0; i < sizeof(DataRelocation); i++) { + ptr[i] = ((void**)this)[i]; + } + } ....... ....... class CallRelocation : public Relocation { ........ + void clone_into(void* ptr[], size_t size) const { + guarantee(sizeof(CallRelocation) <= size, "Make _relocbuf bigger!"); + for (unsigned int i = 0; i < sizeof(CallRelocation); i++) { + ptr[i] = ((void**)this)[i]; + } + } ......... ========================== --- [relocInfo.cpp] ========================== +void Relocation::clone_into(void* ptr[], size_t size) const { + ShouldNotReachHere(); +} + ========================== <OR> [3.2]. Remove 'RelocationHolder(Relocation*)' if it is unused! Another proposal - Can we altogether delete this arg-constructor of RelocationHolder (please tell me if I am missing something here) --- [relocInfo.hpp] ========================== - inline RelocationHolder(Relocation* r); // make a copy .......... -inline RelocationHolder::RelocationHolder(Relocation* r) { - // wordwise copy from r (ok if it copies garbage after r) - for (int i = 0; i < _relocbuf_size; i++) { - _relocbuf[i] = ((void**)r)[i]; - } -} ========================== with following changes? --- [src/cpu/sparc/vm/macroAssembler_sparc.hpp] ========================== class AddressLiteral VALUE_OBJ_CLASS_SPEC { ...... RelocationHolder _rspec; ....... - AddressLiteral() : _address(NULL), _rspec(NULL) {} + AddressLiteral() : _address(NULL), _rspec() {} ========================== --- [src/cpu/x86/vm/assembler_x86.cpp] ========================== void Assembler::jmp_literal(address dest, RelocationHolder const& rspec) { ......... - emit_data(disp, rspec.reloc(), call32_operand); + emit_data(disp, rspec, call32_operand); ========================== --- [src/share/vm/code/relocInfo.hpp] ========================== class trampoline_stub_Relocation : public Relocation { ....... static RelocationHolder spec(address static_call) { RelocationHolder rh = newHolder(); return (new (rh) trampoline_stub_Relocation(static_call)); - return (new (rh) trampoline_stub_Relocation(static_call)); + new (rh) trampoline_stub_Relocation(static_call); + return rh; ==========================
28-08-2017
Thanks to review pointers from Kim Barrett, identified following as changes to be done for complete fix for RelocationHolder / Relocation implementation. (Extracts from email threads) [1]. 'changes in operator Relocation::new(), RelocationHolder::RelocationHolder()' [2]. 'RelocationHolder::reloc() related' [3]. 'fixing RelocationHolder(Relocation*)' [3.1]. Adding clone_into() <OR> [3.2]. Remove 'RelocationHolder(Relocation*)' if it is unused! [1]. 'changes in operator Relocation::new(), RelocationHolder::RelocationHolder()' > There are several problems here: > void* operator new(size_t size, const RelocationHolder& holder) throw() { > if (size > sizeof(holder._relocbuf)) guarantee_size(); > assert((void* const *)holder.reloc() == &holder._relocbuf[0], "ptrs must agree"); > return holder.reloc(); > } > This is presently called in two very different contexts. > (1) RelocationHolder rh = newHolder(); new (rh) <relocation_class>(...); > In this case, the holder is a fully constructed RelocationHolder > object, with holder.reloc() returning a pointer to a fully constructed > Relocation object. Using that pointer as-is will result in > constructing the new Relocation over the existing one, which is > undefined behavior. The old object should be destroyed before reusing > the memory for the new object. > (2) new (*this) Relocation() -- in RelocationHolder() > In this case, the holder is being constructed. holder.reloc() is > returning uninitialized memory cast to Relocation*. > > In (1) we need Relocation::operator new() to call the destructor for > the object returned by holder.reloc(). In (2) we can't call that > destructor because there's no object there to destroy. > My suggestion for (2) is that RelocationHolder() be changed to > RelocationHolder::RelocationHolder() { > STATIC_ASSERT(sizeof(Relocation) <= sizeof(_relocbuf)); > new (_relocbuf) Relocation(); > } > To then deal with (1), change the end of Relocation::operator new() to > holder.reloc()->~Relocation(); > return holder._relocbuf; > [2]. 'RelocationHolder::reloc() related' > Another problem in that operator new definition is that > RelocationHolder::reloc() is silently const-discarding, which is > itself problematic. So we have a const holder argument that is having > it's internals modified out from under it, which is bad. > I suggest there should be two members: > Relocation* reloc() { return (Relocation*)_relocbuf; } > const Relocation* reloc() const { return (const Relocation*)_relocbuf; } > and fix Relocation::operator new() to have the holder parameter be a > non-const reference rather than a const reference. > I haven't looked to see if there are other places that would be > affected by proper const handling by reloc(), so there might be > additional changes needed with that. > [3]. 'fixing RelocationHolder(Relocation*)' [3.1]. Adding clone_into() > We still have the problem of RelocationHolder(Relocation*). This is > presently assuming (1) all classes derived from Relocation are trivialy > copyable, and (2) it is okay to read beyond the end of a Relocation > object. The first might be true, but isn't a good assumption. The > second is not true. > To deal with this, I suggest adding a clone_into virtual function to > Relocation, e.g. something like > virtual void clone_into(void* ptr, size_t size) const. > Each subclass implements this as a placement new copy construct of > itself into ptr, after first verifying that size >= sizeof the > subclass. Then RelocationHolder(Relocation* reloc) calls > reloc->clone_into(_relocbuf, sizeof(_relocbuf)); <OR> [3.2]. Remove 'RelocationHolder(Relocation*)' if it is unused! Another proposal - Can we altogether delete this arg-constructor of RelocationHolder if it is unused!
28-08-2017
Hi Rahul, please have a look if your time allows. Should be straight forward to fix. Thanks, Tobias
22-05-2017
ILW = Potential copy of unaccessible memory, never showed up, no workaround = MLH = P4
22-05-2017
Yes, I agree that copying after 'r' is a bug. Converting.
22-05-2017
Isn't copying after 'r' a bug?
29-06-2016
An earlier version of the report described what I mistakenly thought was a bug.
28-06-2016
[~kbarrett] Isn't it an enhancement?
28-06-2016