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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.

If there are no regressions, we don't need to change libtool_VERSION.

-- 
H.J.