> -----Original Message----- > From: H.J. Lu [mailto:hjl.to...@gmail.com] > Sent: Thursday, November 9, 2017 2:37 PM > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > Cc: Jeff Law <l...@redhat.com>; gcc-patches@gcc.gnu.org; > trie...@redhat.com; Jakub Jelinek <ja...@redhat.com> > Subject: Re: [PATCH 21/22] Add extra field to gtm_jmpbuf on x86 only > > On Wed, Nov 8, 2017 at 2:57 PM, H.J. Lu <hjl.to...@gmail.com> wrote: > > On Wed, Nov 8, 2017 at 2:26 PM, Tsimbalist, Igor V > > <igor.v.tsimbal...@intel.com> wrote: > >>> -----Original Message----- > >>> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > >>> ow...@gcc.gnu.org] On Behalf Of Jeff Law > >>> Sent: Wednesday, November 8, 2017 7:31 PM > >>> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; gcc- > >>> patc...@gcc.gnu.org > >>> Cc: trie...@redhat.com; Jakub Jelinek <ja...@redhat.com> > >>> Subject: Re: [PATCH 21/22] Add extra field to gtm_jmpbuf on x86 only > >>> > >>> On 11/07/2017 09:22 AM, Tsimbalist, Igor V wrote: > >>> > I decided to split my previous patch "Enable building libitm with Intel > CET " > >>> > into two different patches. The first patch will add a new field to > >>> > sjlj.S > and > >>> > target.h files. The second one will add Intel CET support on the top of > the > >>> > first one. In this case the further changes for adding Intel CET support > are > >>> > seen clearly. > >>> > > >>> > Ok for trunk? > >>> > > >>> > >>> [ ... snip ... ] > >>> > >>> > > >>> > > >>> > 0021-Add-extra-field-to-gtm_jmpbuf-on-x86-only.patch > >>> > > >>> > > >>> > From a6361c78bf774f2b4dbeeaf4147c286cff4ae5a4 Mon Sep 17 > 00:00:00 > >>> 2001 > >>> > From: Igor Tsimbalist <igor.v.tsimbal...@intel.com> > >>> > Date: Tue, 7 Nov 2017 17:00:24 +0300 > >>> > Subject: [PATCH 21/22] Add extra field to gtm_jmpbuf on x86 only > >>> > > >>> > Expand the gtm_jmpbuf structure by one word field to add > >>> > Intel CET support further. The code in sjlj.S already > >>> > allocates more space on the stack then gtm_jmpbuf needs. > >>> > Use this extra space to absorb the new field. > >>> > > >>> > The structure is allocated on the stack in such a way > >>> > that eip/rsp field is overlapped with return address on > >>> > the stack. Locate the new field right before eip/rsp so > >>> > code that accesses buffer fields relative to address of > >>> > gtm_jmpbuf has its offsets unchanged. > >>> > > >>> > The libtool_VERSION is updated for x86 due to extending > >>> > the gtm_jmpbuf structure. > >>> > > >>> > * libitm/config/x86/target.h: Add new field (ssp). > >>> > * libitm/config/x86/sjlj.S: Change offsets. > >>> > * libitm/configure.tgt: Update libtool_VERSION. > >>> So if I understand correctly, given the desire to to have the eip/rip > >>> field overlap with the return address on the stack offset changes are > >>> inevitable if we add fields. > >> > >> Yes, that's exactly the case. > >> > >>> > esac > >>> > + > >>> > +# Update libtool_VERSION since the size of struct gtm_jmpbuf is > >>> > +# changed for x86. > >>> > +case "${host}" in > >>> > + > >>> > + # For x86, we use slots in the TCB head for most of our TLS. > >>> > + # The setup of those slots in beginTransaction can afford to > >>> > + # use the global-dynamic model. > >>> > + i[456]86-*-* | x86_64-*-*) > >>> > + libtool_VERSION=2:0:0 > >>> What's the plan for supporting existing code that may have linked > >>> dynamically against libitm? > >> > >> This should just work. > >> > >>> One approach is to force the distros to carry the old libitm DSO. > >>> > >>> THe other would be to try and support both within the same DSO using > >>> symbol versioning. That would seem to imply that we'd need to the > >>> before/after code to build that single library that supported both. > >>> > >>> Thoughts? Jakub, any interest in chiming in here? > >> > >> My thought is that the buffer is encapsulated in the library, only sjlj.S > >> functions allocate the buffer and access the fields of the buffer, it's > >> sort of a black box. If an app loads the library it will work with the > >> buffer through the library's functions from sjlj.S , which are compiled > >> together. > > > > It isn't the black box since gtm_jmpbuf is used in: > > > > struct gtm_transaction_cp > > { > > gtm_jmpbuf jb; > > size_t undolog_size; > > > > If we don't want to change libtool_VERSION, we need to add symbol > > versioning to libitm.so. But from what I can see, libitm.so wasn't > > designed > > with symbol versioning. Instead, it changes libtool_VERSION when > > ABI is changed. > > > > I was wrong on 2 counts: > > 1. libitm does have symbol versioning. > 2. libitm does look like a black box since there is no installed header > and internal is opaque. > > Igor, please do > > 1. Build libitm with the old gtm_jmpbuf. > 2. Build libitm with the new gtm_jmpbuf and the same libtool_VERSION. > 3. Replace the the old libitm with the new libitm. > 4. Run libitm tetsts in the old libitm build tree.
I did 1. On the top of my workspace with all the fixes I discarded the changes libitm/configure.tgt: Update libtool_VERSION. 2. Built the libitm. The library has the suffix 1.0.0. 3. Checked out the sources right before my fixes in libitm. Libitm has an old gtm_jmpbuf (without new field). 4. Built the libitm and run the tests. 5. Replaced the library with the library from the step 2 and run tests All tests passed in both runs. > If there are no regressions, we don't need to change libtool_VERSION. I will revert the changes regarding libtool_VERSION in libitm/configure.tgt. Igor > -- > H.J.
