<div dir="ltr">Which was flagged in post-commit review about a year later. <a href="https://reviews.llvm.org/D58475">https://reviews.llvm.org/D58475</a><div><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature">~Craig</div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 26, 2021 at 12:04 PM Craig Topper <<a href="mailto:craig.topper@gmail.com">craig.topper@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">There's a typo in this line of X86InstrCompiler.td in llvm 10. It should be shiftMask64 not shiftMask32.<div><br></div><div><table style="border-spacing:0px;border-collapse:collapse;color:rgb(201,209,217);font-family:-apple-system,system-ui,"Segoe UI",Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji";font-size:14px;background-color:rgb(13,17,23)"><tbody style="box-sizing:border-box"><tr style="box-sizing:border-box;background-color:transparent"><td id="gmail-m_3301326694749536810gmail-LC1826" style="box-sizing:border-box;padding:0px 10px;line-height:20px;vertical-align:top;overflow:visible;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,monospace;font-size:12px;white-space:pre-wrap">  // (shift x (and y, 63)) ==> (shift x, y)</td></tr><tr style="box-sizing:border-box"><td id="gmail-m_3301326694749536810gmail-L1827" style="box-sizing:border-box;padding:0px 10px;width:50px;min-width:50px;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,monospace;font-size:12px;line-height:20px;text-align:right;white-space:nowrap;vertical-align:top"></td><td id="gmail-m_3301326694749536810gmail-LC1827" style="box-sizing:border-box;padding:0px 10px;line-height:20px;vertical-align:top;overflow:visible;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,monospace;font-size:12px;white-space:pre-wrap">  def : Pat<(frag GR64:$src1, GR64:$src2, (shiftMask32 CL)),</td></tr><tr style="box-sizing:border-box;background-color:transparent"><td id="gmail-m_3301326694749536810gmail-L1828" style="box-sizing:border-box;padding:0px 10px;width:50px;min-width:50px;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,monospace;font-size:12px;line-height:20px;text-align:right;white-space:nowrap;vertical-align:top"></td><td id="gmail-m_3301326694749536810gmail-LC1828" style="box-sizing:border-box;padding:0px 10px;line-height:20px;vertical-align:top;overflow:visible;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,monospace;font-size:12px;white-space:pre-wrap">            (!cast<Instruction>(name # "64rrCL") GR64:$src1, GR64:$src2)>;</td></tr></tbody></table></div><div><br></div><div><br clear="all"><div><div dir="ltr">~Craig</div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 26, 2021 at 11:57 AM Riyaz Puthiyapurayil via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Curiously, for the following test, with -fsanitize=undefined -O3, clang-10 reports no UB errors and produces the correct run-time result. But without -fsanitize=undefined, it produces wrong output. Clang (trunk) produces correct results either way. I think there is definitely a bug that was fixed recently (in the last year or so). Could it be this one:<br>
<br>
<a href="https://reviews.llvm.org/D75748?id=" rel="noreferrer" target="_blank">https://reviews.llvm.org/D75748?id=</a><br>
<br>
Roman, you were a reviewer for this.<br>
<br>
-----Original Message-----<br>
From: Riyaz Puthiyapurayil <br>
Sent: Tuesday, January 26, 2021 11:48 AM<br>
To: 'Roman Lebedev' <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>><br>
Cc: <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
Subject: RE: [llvm-dev] LLVM 10 is miscompiling this code on x86_64 with an invalid shift count for `shrd`<br>
<br>
My bad! There was a typo that caused the UBSAN error.  Here is the corrected test. This time UBSAN reports no errors. But clang-10 still produces unexpected result whereas trunk does not.<br>
<br>
#include <stdio.h><br>
#include <stdint.h><br>
#include <string.h><br>
<br>
__attribute__ ((noinline)) uint64_t  foo4(__int128_t* arr1) {<br>
    unsigned chunk = 34 >> 5;<br>
    unsigned bitIndex = 34 - chunk * 32;<br>
<br>
    unsigned* arr = (unsigned *)arr1 + chunk;<br>
    __int128_t v1 = *arr1;<br>
    v1 = v1 >> bitIndex;<br>
<br>
    return (uint64_t) v1;<br>
}<br>
<br>
__attribute__ ((noinline)) uint64_t  foo3(__int128_t* arr1, unsigned index) {<br>
    unsigned chunk = index >> 5;<br>
    unsigned bitIndex = index - chunk * 32;<br>
<br>
    unsigned *arr = (unsigned *)arr1 + chunk;<br>
    __int128_t v1 = *arr1;<br>
    v1 = v1 >> bitIndex;<br>
<br>
    return (uint64_t) v1;<br>
}<br>
<br>
int main() {<br>
<br>
    unsigned arr[4];<br>
<br>
    arr[0]= 0x6f7cfd6f;<br>
    arr[1]= 0xd96c9806;<br>
    arr[2]= 0x89420144;<br>
    arr[3]= 0x8a20f548;<br>
<br>
    __int128_t arr1;<br>
<br>
    memcpy(&arr1, arr, sizeof(arr1));<br>
<br>
    printf("foo3 %lx\n",foo3(&arr1,34) & 0x3ffffffff);   <br>
    printf("foo4 %lx\n",foo4(&arr1) & 0x3ffffffff); }<br>
<br>
-----Original Message-----<br>
From: Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>><br>
Sent: Tuesday, January 26, 2021 11:31 AM<br>
To: Riyaz Puthiyapurayil <<a href="mailto:riyaz@synopsys.com" target="_blank">riyaz@synopsys.com</a>><br>
Cc: <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
Subject: Re: [llvm-dev] LLVM 10 is miscompiling this code on x86_64 with an invalid shift count for `shrd`<br>
<br>
<a href="https://urldefense.com/v3/__https://godbolt.org/z/5bcsj9__;!!A4F2R9G_pg!P3wQMqAfQM_jzlQy2F-1LD2x1ekeAKGQckArJiV5UsQIR2NhtjHbH5ivGPqtfl8bZLzgtjq5KIG7$" rel="noreferrer" target="_blank">https://urldefense.com/v3/__https://godbolt.org/z/5bcsj9__;!!A4F2R9G_pg!P3wQMqAfQM_jzlQy2F-1LD2x1ekeAKGQckArJiV5UsQIR2NhtjHbH5ivGPqtfl8bZLzgtjq5KIG7$</a> <br>
<br>
example.c:43:37: runtime error: subtraction of unsigned offset from<br>
0x000000000022 overflowed to 0x7fffd01217c8<br>
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior example.c:43:37 in<br>
example.c:19:37: runtime error: applying non-zero offset<br>
140576899311424 to null pointer<br>
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior example.c:19:37 in<br>
foo3 19bdf3f5b<br>
foo4 19bdf3f5b<br>
<br>
On Tue, Jan 26, 2021 at 10:29 PM Riyaz Puthiyapurayil <<a href="mailto:Riyaz.Puthiyapurayil@synopsys.com" target="_blank">Riyaz.Puthiyapurayil@synopsys.com</a>> wrote:<br>
><br>
> Apologies, outlook messed up the formatting in the previous email. Here is it again.<br>
><br>
><br>
><br>
> Do you see any UB in the following modified test? Here also, I see a difference between foo3 and foo4. Again, trunk and gcc match. But not Clang-10.<br>
><br>
><br>
><br>
> #include <stdio.h><br>
><br>
> #include <stdint.h><br>
><br>
> #include <string.h><br>
><br>
><br>
><br>
> __attribute__ ((noinline)) uint64_t  foo4(__int128_t* arr1)<br>
><br>
> {<br>
><br>
>     unsigned chunk = 34 >> 5;<br>
><br>
>     unsigned bitIndex = 34 - chunk * 32;<br>
><br>
><br>
><br>
>     unsigned* arr = (unsigned *)arr + chunk;<br>
><br>
>     __int128_t v1 = *arr1;<br>
><br>
>     v1 = v1 >> bitIndex;<br>
><br>
><br>
><br>
>     return (uint64_t) v1;<br>
><br>
> }<br>
><br>
><br>
><br>
> __attribute__ ((noinline)) uint64_t  foo3(__int128_t* arr1, unsigned<br>
> index)<br>
><br>
> {<br>
><br>
>     unsigned chunk = index >> 5;<br>
><br>
>     unsigned bitIndex = index - chunk * 32;<br>
><br>
><br>
><br>
>     unsigned *arr = (unsigned *)arr + chunk;<br>
><br>
>     __int128_t v1 = *arr1;<br>
><br>
>     v1 = v1 >> bitIndex;<br>
><br>
><br>
><br>
>     return (uint64_t) v1;<br>
><br>
> }<br>
><br>
><br>
><br>
> int main() {<br>
><br>
><br>
><br>
>     unsigned arr[4];<br>
><br>
><br>
><br>
>     arr[0]= 0x6f7cfd6f;<br>
><br>
>     arr[1]= 0xd96c9806;<br>
><br>
>     arr[2]= 0x89420144;<br>
><br>
>     arr[3]= 0x8a20f548;<br>
><br>
><br>
><br>
>     __int128_t arr1;<br>
><br>
><br>
><br>
>     memcpy(&arr1, arr, sizeof(arr1));<br>
><br>
><br>
><br>
>     printf("foo3 %lx\n",foo3(&arr1,34) & 0x3ffffffff);<br>
><br>
>     printf("foo4 %lx\n",foo4(&arr1) & 0x3ffffffff);<br>
><br>
> }<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> From: Riyaz Puthiyapurayil<br>
> Sent: Tuesday, January 26, 2021 11:25 AM<br>
> To: 'Roman Lebedev' <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>><br>
> Cc: <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> Subject: RE: [llvm-dev] LLVM 10 is miscompiling this code on x86_64 <br>
> with an invalid shift count for `shrd`<br>
><br>
><br>
><br>
> Do you see any UB in the following modified test? Here also, I see a <br>
> difference between foo3 and foo4. Again, trunk and gcc match. But not<br>
> Clang-10<br>
><br>
><br>
><br>
> #include <stdio.h><br>
><br>
> #include <stdint.h><br>
><br>
> #include <string.h><br>
><br>
><br>
><br>
> __attribute__ ((noinline)) uint64_t  foo4(__int128_t* arr1)<br>
><br>
> {<br>
><br>
>     unsigned chunk = 34 >> 5;<br>
><br>
>     unsigned bitIndex = 34 - chunk * 32;<br>
><br>
><br>
><br>
>     unsigned* arr = (unsigned *)arr + chunk;<br>
><br>
>     __int128_t v1 = *arr1;<br>
><br>
>     v1 = v1 >> bitIndex;<br>
><br>
><br>
><br>
>     return (uint64_t) v1;<br>
><br>
> }<br>
><br>
><br>
><br>
> __attribute__ ((noinline)) uint64_t  foo3(__int128_t* arr1, unsigned<br>
> index)<br>
><br>
> {<br>
><br>
>     unsigned chunk = index >> 5;<br>
><br>
>     unsigned bitIndex = index - chunk * 32;<br>
><br>
><br>
><br>
>     unsigned *arr = (unsigned *)arr + chunk;<br>
><br>
>     __int128_t v1 = *arr1;<br>
><br>
>     v1 = v1 >> bitIndex;<br>
><br>
><br>
><br>
>     return (uint64_t) v1;<br>
><br>
> }<br>
><br>
><br>
><br>
> int main() {<br>
><br>
><br>
><br>
>     unsigned arr[4];<br>
><br>
><br>
><br>
>     arr[0]= 0x6f7cfd6f;<br>
><br>
>     arr[1]= 0xd96c9806;<br>
><br>
>     arr[2]= 0x89420144;<br>
><br>
>     arr[3]= 0x8a20f548;<br>
><br>
><br>
><br>
>     __int128_t arr1;<br>
><br>
><br>
><br>
>     memcpy(&arr1, arr, sizeof(arr1));<br>
><br>
><br>
><br>
>     printf("foo3 %lx\n",foo3(&arr1,34) & 0x3ffffffff); // prints<br>
> 1365b2601 (clang-10) 19bdf3f5b (gcc, clang-12)<br>
><br>
>     printf("foo4 %lx\n",foo4(&arr1) & 0x3ffffffff);    // prints 19bdf3f5b<br>
><br>
> }<br>
><br>
><br>
><br>
> -----Original Message-----<br>
> From: Riyaz Puthiyapurayil<br>
> Sent: Tuesday, January 26, 2021 11:10 AM<br>
> To: 'Roman Lebedev' <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>><br>
> Subject: RE: [llvm-dev] LLVM 10 is miscompiling this code on x86_64 <br>
> with an invalid shift count for `shrd`<br>
><br>
><br>
><br>
> You may be right about that the cast from unsigned* to __int128_t* being UB. But I think that has nothing to do with the bug. That code can be fixed to use a __int128_t array and change all the casts from __int128_t* to unsigned* (instead of the other way around) and we would still have the issue.<br>
><br>
><br>
><br>
> -----Original Message-----<br>
><br>
> From: Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>><br>
><br>
> Sent: Tuesday, January 26, 2021 10:34 AM<br>
><br>
> To: Riyaz Puthiyapurayil <<a href="mailto:riyaz@synopsys.com" target="_blank">riyaz@synopsys.com</a>><br>
><br>
> Cc: <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
><br>
> Subject: Re: [llvm-dev] LLVM 10 is miscompiling this code on x86_64 <br>
> with an invalid shift count for `shrd`<br>
><br>
><br>
><br>
> On Tue, Jan 26, 2021 at 9:28 PM Riyaz Puthiyapurayil via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
> ><br>
><br>
> > It appears that LLVM 10 is miscompiling the following test on x86_64 but the trunk version seems to work fine. Before I spend any time debugging this, it would be helpful if someone can point me to a bug fix if there was one.<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> > #include <stdio.h><br>
><br>
> ><br>
><br>
> > #include <stdint.h><br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> > __attribute__ ((noinline)) uint64_t  foo4(unsigned* arr)<br>
><br>
> ><br>
><br>
> > {<br>
><br>
> ><br>
><br>
> >     unsigned chunk = 34 >> 5;<br>
><br>
> ><br>
><br>
> >     unsigned bitIndex = 34 - chunk * 32;<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> >     arr = arr + chunk;<br>
><br>
> ><br>
><br>
> >     __int128_t* arr1 = (__int128_t*) arr;<br>
><br>
> I'm pretty sure this is UB, because the alignment of __int128_t is bigger than that of unsigned.<br>
><br>
><br>
><br>
> ><br>
><br>
> >     __int128_t v1 = *arr1;<br>
><br>
> ><br>
><br>
> >     v1 = v1 >> bitIndex;<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> >     return (uint64_t) v1;<br>
><br>
> ><br>
><br>
> > }<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> > __attribute__ ((noinline)) uint64_t  foo3(unsigned* arr, unsigned<br>
><br>
> > index)<br>
><br>
> ><br>
><br>
> > {<br>
><br>
> ><br>
><br>
> >     unsigned chunk = index >> 5;<br>
><br>
> ><br>
><br>
> >     unsigned bitIndex = index - chunk * 32;<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> >     arr = arr + chunk;<br>
><br>
> ><br>
><br>
> >     __int128_t* arr1 = (__int128_t*) arr;<br>
><br>
> ><br>
><br>
> >     __int128_t v1 = *arr1;<br>
><br>
> ><br>
><br>
> >     v1 = v1 >> bitIndex;<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> >     return (uint64_t) v1;<br>
><br>
> ><br>
><br>
> > }<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> > int main() {<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> >     unsigned arr[5];<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> >     arr[0]= 0x6f7cfd6f;<br>
><br>
> ><br>
><br>
> >     arr[1]= 0xd96c9806;<br>
><br>
> ><br>
><br>
> >     arr[2]= 0x89420144;<br>
><br>
> ><br>
><br>
> >     arr[3]= 0x8a20f548;<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> >     printf("foo3 %lx\n",foo3(arr,34) & 0x3ffffffff); // prints<br>
><br>
> > 222508051 instead of 1365b2601<br>
><br>
> ><br>
><br>
> >     printf("foo4 %lx\n",foo4(arr) & 0x3ffffffff); // prints<br>
> > 1365b2601<br>
><br>
> > as expected<br>
><br>
> ><br>
><br>
> > }<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> > If you look at the assembly, there is an ‘and cl, 31’ missing in foo3 which is needed to make sure that the shift count is in the range [0...31]. This is missing in the generated code. Note that if the count in CL register is greater than 31, x86_64 manual states that the behavior of shrd is undefined.<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> > # clang 10<br>
><br>
> ><br>
><br>
> > foo3(unsigned int*, unsigned int):                             # @foo3(unsigned int*, unsigned int)<br>
><br>
> ><br>
><br>
> >         mov     ecx, esi<br>
><br>
> ><br>
><br>
> >         mov     edx, esi<br>
><br>
> ><br>
><br>
> >         shr     edx, 5<br>
><br>
> ><br>
><br>
> >         mov     rax, qword ptr [rdi + 4*rdx]<br>
><br>
> ><br>
><br>
> >         mov     rdx, qword ptr [rdi + 4*rdx + 8]<br>
><br>
> ><br>
><br>
> >         shrd    rax, rdx, cl<br>
><br>
> ><br>
><br>
> >         ret<br>
><br>
> ><br>
><br>
> ><br>
><br>
> ><br>
><br>
> > # Trunk<br>
><br>
> ><br>
><br>
> > foo3(unsigned int*, unsigned int):                             # @foo3(unsigned int*, unsigned int)<br>
><br>
> ><br>
><br>
> >         mov     ecx, esi<br>
><br>
> ><br>
><br>
> >         mov     edx, esi<br>
><br>
> ><br>
><br>
> >         shr     edx, 5<br>
><br>
> ><br>
><br>
> >         mov     rax, qword ptr [rdi + 4*rdx]<br>
><br>
> ><br>
><br>
> >         mov     rdx, qword ptr [rdi + 4*rdx + 8]<br>
><br>
> ><br>
><br>
> >         and     cl, 31<br>
><br>
> ><br>
><br>
> >         shrd    rax, rdx, cl<br>
><br>
> ><br>
><br>
> >         ret<br>
><br>
> ><br>
><br>
><br>
><br>
> Roman.<br>
><br>
><br>
><br>
> > _______________________________________________<br>
><br>
> > LLVM Developers mailing list<br>
><br>
> > <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
><br>
> > <a href="https://urldefense.com/v3/__https://lists.llvm.org/cgi-bin/mailman/l" rel="noreferrer" target="_blank">https://urldefense.com/v3/__https://lists.llvm.org/cgi-bin/mailman/l</a><br>
> > is<br>
><br>
> > tinfo/llvm-dev__;!!A4F2R9G_pg!Mq1ZgXCY7kwDMwNwdn4Tcz-KgkoXMlP_rjq7_U<br>
> > 9M<br>
><br>
> > rh1gbrtqvDoSi6cMuSLZaXIPgtijPW55JJ_8$<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
</blockquote></div>