On 11/13/2017 06:53 AM, Tsimbalist, Igor V wrote: >> -----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. FWIW, I wandered through libitm a bit and I think gtm_jmpbuf is supposed to be an internal implementation detail. I don't see references to it or any structure that contains a gtm_jmpbuf in libitm.h. So I don't see any direct way for it to escape.
I think with that analysis as well as your testing we can just drop the chunk that changes the version and the rest is OK. Jeff