Re: [PATCH 21/22] Add extra field to gtm_jmpbuf on x86 only

[Jeff Law <law at redhat dot com>]

On 11/13/2017 06:53 AM, Tsimbalist, Igor V wrote:
>> -----Original Message-----
>> From: H.J. Lu [mailto:hjl.tools@gmail.com]
>> Sent: Thursday, November 9, 2017 2:37 PM
>> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
>> Cc: Jeff Law <law@redhat.com>; gcc-patches@gcc.gnu.org;
>> triegel@redhat.com; Jakub Jelinek <jakub@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.tools@gmail.com> wrote:
>>> On Wed, Nov 8, 2017 at 2:26 PM, Tsimbalist, Igor V
>>> <igor.v.tsimbalist@intel.com> wrote:
>>>>> -----Original Message-----
>>>>> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
>>>>> owner@gcc.gnu.org] On Behalf Of Jeff Law
>>>>> Sent: Wednesday, November 8, 2017 7:31 PM
>>>>> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc-
>>>>> patches@gcc.gnu.org
>>>>> Cc: triegel@redhat.com; Jakub Jelinek <jakub@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.tsimbalist@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