Clang + AArch64 + non-ABI workaround

Message ID fc056372-18d3-4608-19ed-4699c2c4d12d@linaro.org
State New
Headers show
Series
  • Clang + AArch64 + non-ABI workaround
Related show

Commit Message

Dmitry Eremin-Solenikov Feb. 16, 2018, 2:54 p.m.
Hello,

I've been debugging the Clang/AArch64/non-ABI case during this week.

It indeed is a compiler issue. Here is a workaround, which fixes the
issue for at least clang 7 (did not try with earlier versions, probably
it would also help). At this moment I do not think we should apply this
fix, but rather declare this combination as not supported. Our code is
perfectly valid from my POV.

                burst     = ring_deq_multi(ring, mask, data, burst);


-- 
With best wishes
Dmitry

Comments

Maxim Uvarov Feb. 16, 2018, 3:26 p.m. | #1
can you link to problem description? is it segfault here?

On 16 February 2018 at 17:54, Dmitry Eremin-Solenikov <
dmitry.ereminsolenikov@linaro.org> wrote:

> Hello,

>

> I've been debugging the Clang/AArch64/non-ABI case during this week.

>

> It indeed is a compiler issue. Here is a workaround, which fixes the

> issue for at least clang 7 (did not try with earlier versions, probably

> it would also help). At this moment I do not think we should apply this

> fix, but rather declare this combination as not supported. Our code is

> perfectly valid from my POV.

>

> diff --git a/platform/linux-generic/odp_pool.c

> b/platform/linux-generic/odp_pool.c

> index e5ba8982a29a..a97d096d1a53 100644

> --- a/platform/linux-generic/odp_pool.c

> +++ b/platform/linux-generic/odp_pool.c

> @@ -727,12 +727,13 @@ int buffer_alloc_multi(pool_t *pool,

> odp_buffer_hdr_t *buf_hdr[], int max_num)

>                 buf_hdr[i] = buf_hdr_from_index(pool, cache->buf_index[j]);

>         }

>

> -       /* If needed, get more from the global pool */

> -       if (odp_unlikely(num_deq)) {

>                 /* Temporary copy needed since odp_buffer_t is uintptr_t

>                  * and not uint32_t. */

>                 uint32_t data[burst];

>

> +       /* If needed, get more from the global pool */

> +       if (odp_unlikely(num_deq)) {

> +

>                 ring      = &pool->ring->hdr;

>                 mask      = pool->ring_mask;

>                 burst     = ring_deq_multi(ring, mask, data, burst);

>

>

> --

> With best wishes

> Dmitry

>
Dmitry Eremin-Solenikov Feb. 16, 2018, 6:47 p.m. | #2
On 16 February 2018 at 18:26, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> can you link to problem description? is it segfault here?


https://bugs.linaro.org/show_bug.cgi?id=3611

>

> On 16 February 2018 at 17:54, Dmitry Eremin-Solenikov

> <dmitry.ereminsolenikov@linaro.org> wrote:

>>

>> Hello,

>>

>> I've been debugging the Clang/AArch64/non-ABI case during this week.

>>

>> It indeed is a compiler issue. Here is a workaround, which fixes the

>> issue for at least clang 7 (did not try with earlier versions, probably

>> it would also help). At this moment I do not think we should apply this

>> fix, but rather declare this combination as not supported. Our code is

>> perfectly valid from my POV.

>>

>> diff --git a/platform/linux-generic/odp_pool.c

>> b/platform/linux-generic/odp_pool.c

>> index e5ba8982a29a..a97d096d1a53 100644

>> --- a/platform/linux-generic/odp_pool.c

>> +++ b/platform/linux-generic/odp_pool.c

>> @@ -727,12 +727,13 @@ int buffer_alloc_multi(pool_t *pool,

>> odp_buffer_hdr_t *buf_hdr[], int max_num)

>>                 buf_hdr[i] = buf_hdr_from_index(pool,

>> cache->buf_index[j]);

>>         }

>>

>> -       /* If needed, get more from the global pool */

>> -       if (odp_unlikely(num_deq)) {

>>                 /* Temporary copy needed since odp_buffer_t is uintptr_t

>>                  * and not uint32_t. */

>>                 uint32_t data[burst];

>>

>> +       /* If needed, get more from the global pool */

>> +       if (odp_unlikely(num_deq)) {

>> +

>>                 ring      = &pool->ring->hdr;

>>                 mask      = pool->ring_mask;

>>                 burst     = ring_deq_multi(ring, mask, data, burst);

>>

>>

>> --

>> With best wishes

>> Dmitry

>

>




-- 
With best wishes
Dmitry
Maxim Uvarov Feb. 16, 2018, 7:17 p.m. | #3
On 02/16/18 21:47, Dmitry Eremin-Solenikov wrote:
> On 16 February 2018 at 18:26, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>> can you link to problem description? is it segfault here?

> 

> https://bugs.linaro.org/show_bug.cgi?id=3611

> 


but it has to fail.
as I said /dev/shm under docker (which uses shippable) is limited to
64Mb by default.

we run: ODP_SHM_DIR=/dev/shm/odp make check

So it creates files (alloc shared memory) in that directory. Helpers
like cuckoohash use more then 64Mb memory to build hash table. It
allocates memory but on access there is seg. fault.

I think ODP_SHM_DIR=/var make check has to work here.

But it's not clear how it's related to this patch.

Maxim.

>>

>> On 16 February 2018 at 17:54, Dmitry Eremin-Solenikov

>> <dmitry.ereminsolenikov@linaro.org> wrote:

>>>

>>> Hello,

>>>

>>> I've been debugging the Clang/AArch64/non-ABI case during this week.

>>>

>>> It indeed is a compiler issue. Here is a workaround, which fixes the

>>> issue for at least clang 7 (did not try with earlier versions, probably

>>> it would also help). At this moment I do not think we should apply this

>>> fix, but rather declare this combination as not supported. Our code is

>>> perfectly valid from my POV.

>>>

>>> diff --git a/platform/linux-generic/odp_pool.c

>>> b/platform/linux-generic/odp_pool.c

>>> index e5ba8982a29a..a97d096d1a53 100644

>>> --- a/platform/linux-generic/odp_pool.c

>>> +++ b/platform/linux-generic/odp_pool.c

>>> @@ -727,12 +727,13 @@ int buffer_alloc_multi(pool_t *pool,

>>> odp_buffer_hdr_t *buf_hdr[], int max_num)

>>>                 buf_hdr[i] = buf_hdr_from_index(pool,

>>> cache->buf_index[j]);

>>>         }

>>>

>>> -       /* If needed, get more from the global pool */

>>> -       if (odp_unlikely(num_deq)) {

>>>                 /* Temporary copy needed since odp_buffer_t is uintptr_t

>>>                  * and not uint32_t. */

>>>                 uint32_t data[burst];

>>>

>>> +       /* If needed, get more from the global pool */

>>> +       if (odp_unlikely(num_deq)) {

>>> +

>>>                 ring      = &pool->ring->hdr;

>>>                 mask      = pool->ring_mask;

>>>                 burst     = ring_deq_multi(ring, mask, data, burst);

>>>

>>>

>>> --

>>> With best wishes

>>> Dmitry

>>

>>

> 

> 

>
Dmitry Eremin-Solenikov Feb. 16, 2018, 7:42 p.m. | #4
Hi,

On 16 February 2018 at 22:17, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> On 02/16/18 21:47, Dmitry Eremin-Solenikov wrote:

>> On 16 February 2018 at 18:26, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

>>> can you link to problem description? is it segfault here?

>>

>> https://bugs.linaro.org/show_bug.cgi?id=3611

>>

>

> but it has to fail.

> as I said /dev/shm under docker (which uses shippable) is limited to

> 64Mb by default.

>

> we run: ODP_SHM_DIR=/dev/shm/odp make check

>

> So it creates files (alloc shared memory) in that directory. Helpers

> like cuckoohash use more then 64Mb memory to build hash table. It

> allocates memory but on access there is seg. fault.

>

> I think ODP_SHM_DIR=/var make check has to work here.


If you check, segfaults happen before creating MBs of data. It happens
early (and in plenty of tests actually) because of errors during buffer
allocation. I've traced today the issue to Clang's compiled
buffer_alloc_muli incorrectly shifting SP register if data[] array is
allocated inside if () branch. Somehow this happens only
in non-ABI-compat case.

> But it's not clear how it's related to this patch.

>

> Maxim.

>

>>>

>>> On 16 February 2018 at 17:54, Dmitry Eremin-Solenikov

>>> <dmitry.ereminsolenikov@linaro.org> wrote:

>>>>

>>>> Hello,

>>>>

>>>> I've been debugging the Clang/AArch64/non-ABI case during this week.

>>>>

>>>> It indeed is a compiler issue. Here is a workaround, which fixes the

>>>> issue for at least clang 7 (did not try with earlier versions, probably

>>>> it would also help). At this moment I do not think we should apply this

>>>> fix, but rather declare this combination as not supported. Our code is

>>>> perfectly valid from my POV.

>>>>

>>>> diff --git a/platform/linux-generic/odp_pool.c

>>>> b/platform/linux-generic/odp_pool.c

>>>> index e5ba8982a29a..a97d096d1a53 100644

>>>> --- a/platform/linux-generic/odp_pool.c

>>>> +++ b/platform/linux-generic/odp_pool.c

>>>> @@ -727,12 +727,13 @@ int buffer_alloc_multi(pool_t *pool,

>>>> odp_buffer_hdr_t *buf_hdr[], int max_num)

>>>>                 buf_hdr[i] = buf_hdr_from_index(pool,

>>>> cache->buf_index[j]);

>>>>         }

>>>>

>>>> -       /* If needed, get more from the global pool */

>>>> -       if (odp_unlikely(num_deq)) {

>>>>                 /* Temporary copy needed since odp_buffer_t is uintptr_t

>>>>                  * and not uint32_t. */

>>>>                 uint32_t data[burst];

>>>>

>>>> +       /* If needed, get more from the global pool */

>>>> +       if (odp_unlikely(num_deq)) {

>>>> +

>>>>                 ring      = &pool->ring->hdr;

>>>>                 mask      = pool->ring_mask;

>>>>                 burst     = ring_deq_multi(ring, mask, data, burst);

>>>>

>>>>

>>>> --

>>>> With best wishes

>>>> Dmitry

>>>

>>>

>>

>>

>>

>




-- 
With best wishes
Dmitry

Patch

diff --git a/platform/linux-generic/odp_pool.c
b/platform/linux-generic/odp_pool.c
index e5ba8982a29a..a97d096d1a53 100644
--- a/platform/linux-generic/odp_pool.c
+++ b/platform/linux-generic/odp_pool.c
@@ -727,12 +727,13 @@  int buffer_alloc_multi(pool_t *pool,
odp_buffer_hdr_t *buf_hdr[], int max_num)
                buf_hdr[i] = buf_hdr_from_index(pool, cache->buf_index[j]);
        }

-       /* If needed, get more from the global pool */
-       if (odp_unlikely(num_deq)) {
                /* Temporary copy needed since odp_buffer_t is uintptr_t
                 * and not uint32_t. */
                uint32_t data[burst];

+       /* If needed, get more from the global pool */
+       if (odp_unlikely(num_deq)) {
+
                ring      = &pool->ring->hdr;
                mask      = pool->ring_mask;