| Message ID | fc056372-18d3-4608-19ed-4699c2c4d12d@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Clang + AArch64 + non-ABI workaround | expand |
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 >
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
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 >> >> > > >
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
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;
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