<div dir="ltr"><div dir="ltr">On Fri, Apr 26, 2019 at 3:33 PM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<p>This doesn't feel like a case where we should change clang's
behavior to be more conservative by default. Maybe adding a
"const sanitizer" or an -fallow-const-misuse might be justified,
but I'm not sure I'd go any further that that.</p></div></blockquote><div>FWIW, we just patched busybox to not have the const.</div><div><br></div><div>As stated originally, I sent this mostly in case many more folks hit these issues and want to push for a more fundamental strategy. So far I've not seen much if any cause.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><p> <br>
</p>
<p>If we did want to sanitize for such inputs, replacing the
instcombine rule I added with an assertion of the same assumption
would be pretty straight-forward. I haven't worked on the
sanatizers, anyone know if there's a natural place to fit such a
check?</p></div></blockquote><div>If UBSan doesn't always catch this, please file a bug. It should.</div><div><br></div><div>This is a UBSan-style thing -- it might require some minor help from LLVM but generally should be a trivial and local check.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<p>Philip<br>
</p>
<div class="gmail-m_-2674758125382800241moz-cite-prefix">On 4/25/19 12:58 PM, Eli Friedman
wrote:<br>
</div>
<blockquote type="cite">
<div class="gmail-m_-2674758125382800241WordSection1">
<p class="MsoNormal">We had a bug reported about busybox getting
miscompiled due to the incorrect use of “const” before; see
<a href="https://bugs.llvm.org/show_bug.cgi?id=39919" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=39919</a>
. In that case, we weren’t removing the store; we just
rescheduled it after a load from the same variable. The
comments there indicate a bug was reported against busybox,
but I don’t know what happened after that.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">-Eli<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<div style="border-top:none;border-right:none;border-bottom:none;border-left:1.5pt solid blue;padding:0in 0in 0in 4pt">
<div>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(225,225,225);padding:3pt 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-commits
<a class="gmail-m_-2674758125382800241moz-txt-link-rfc2396E" href="mailto:llvm-commits-bounces@lists.llvm.org" target="_blank"><llvm-commits-bounces@lists.llvm.org></a>
<b>On Behalf Of </b>Chandler Carruth via llvm-commits<br>
<b>Sent:</b> Thursday, April 25, 2019 10:28 AM<br>
<b>To:</b> Philip Reames
<a class="gmail-m_-2674758125382800241moz-txt-link-rfc2396E" href="mailto:listmail@philipreames.com" target="_blank"><listmail@philipreames.com></a>; Richard Smith
<a class="gmail-m_-2674758125382800241moz-txt-link-rfc2396E" href="mailto:richard@metafoo.co.uk" target="_blank"><richard@metafoo.co.uk></a><br>
<b>Cc:</b> llvm-commits
<a class="gmail-m_-2674758125382800241moz-txt-link-rfc2396E" href="mailto:llvm-commits@lists.llvm.org" target="_blank"><llvm-commits@lists.llvm.org></a><br>
<b>Subject:</b> [EXT] Re: [llvm] r358919 - [InstCombine]
Eliminate stores to constant memory<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">I wanted to write this so people were
aware this patch will break code that exists in the wild.<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Consider code like this from BusyBox:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><a href="https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L216" target="_blank">https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L216</a><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span class="gmail-m_-2674758125382800241gmail-pl-k"><span style="font-size:9pt;font-family:Consolas;color:rgb(215,58,73)">extern</span></span><span style="font-size:9pt;font-family:Consolas;color:rgb(36,41,46);background:rgb(255,251,221)">
</span><span class="gmail-m_-2674758125382800241gmail-pl-k"><span style="font-size:9pt;font-family:Consolas;color:rgb(215,58,73)">struct</span></span><span style="font-size:9pt;font-family:Consolas;color:rgb(36,41,46);background:rgb(255,251,221)">
globals_misc *</span><span class="gmail-m_-2674758125382800241gmail-pl-k"><span style="font-size:9pt;font-family:Consolas;color:rgb(215,58,73)">const</span></span><span style="font-size:9pt;font-family:Consolas;color:rgb(36,41,46);background:rgb(255,251,221)">
ash_ptr_to_globals_misc;</span><u></u><u></u></p>
</div>
<p class="MsoNormal"><br>
The code goes on to cast away the const and store to this
global:<u></u><u></u></p>
<div>
<p class="MsoNormal"><a href="https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L241" target="_blank">https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L241</a><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I believe your patch is entirely
correct. I believe Clang is correct to mark this memory
as constant. I just wanted people to be aware that code
in the wild doesn't abide by these rules.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">If we want to do something about it,
I believe we should change Clang to not mark *extern*
const globals as constant memory. <a href="mailto:richard@metafoo.co.uk" target="_blank">+Richard Smith</a> for
confirmation on that approach. Maybe behind a flag or
only in certain modes.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">So far, this is the only example I
have found however, and so it may be sufficiently rare
to not warrant doing anything about it. Sending this
email in case others are debugging similar issues, hit
this revision, and add information that indicates it is
not as rare as I am hoping.<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">On Mon, Apr 22, 2019 at 1:26 PM
Philip Reames via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>
wrote:<u></u><u></u></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">Author: reames<br>
Date: Mon Apr 22 13:28:19 2019<br>
New Revision: 358919<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=358919&view=rev" target="_blank">
http://llvm.org/viewvc/llvm-project?rev=358919&view=rev</a><br>
Log:<br>
[InstCombine] Eliminate stores to constant memory<br>
<br>
If we have a store to a piece of memory which is known
constant, then we know the store must be storing back
the same value. As a result, the store (or memset, or
memmove) must either be down a dead path, or a noop. In
either case, it is valid to simply remove the store.<br>
<br>
The motivating case for this involves a memmove to a
buffer which is constant down a path which is
dynamically dead.<br>
<br>
Note that I'm choosing to implement the less aggressive
of two possible semantics here. We could simply say that
the store *is undefined*, and prune the path. Consensus
in the review was that the more aggressive form might be
a good follow on change at a later date.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D60659" target="_blank">
https://reviews.llvm.org/D60659</a><br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp<br>
llvm/trunk/test/Transforms/InstCombine/memcpy.ll<br>
llvm/trunk/test/Transforms/InstCombine/memmove.ll<br>
llvm/trunk/test/Transforms/InstCombine/memset.ll<br>
llvm/trunk/test/Transforms/InstCombine/store.ll<br>
<br>
Modified:
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
---
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
(original)<br>
+++
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
Mon Apr 22 13:28:19 2019<br>
@@ -120,6 +120,15 @@ Instruction
*InstCombiner::SimplifyAnyMe<br>
return MI;<br>
}<br>
<br>
+ // If we have a store to a location which is known
constant, we can conclude<br>
+ // that the store must be storing the constant value
(else the memory<br>
+ // wouldn't be constant), and this must be a noop.<br>
+ if (AA->pointsToConstantMemory(MI->getDest()))
{<br>
+ // Set the size of the copy to 0, it will be
deleted on the next iteration.<br>
+
MI->setLength(Constant::getNullValue(MI->getLength()->getType()));<br>
+ return MI;<br>
+ }<br>
+<br>
// If MemCpyInst length is 1/2/4/8 bytes then replace
memcpy with<br>
// load/store.<br>
ConstantInt *MemOpLength =
dyn_cast<ConstantInt>(MI->getLength());<br>
@@ -218,6 +227,15 @@ Instruction
*InstCombiner::SimplifyAnyMe<br>
return MI;<br>
}<br>
<br>
+ // If we have a store to a location which is known
constant, we can conclude<br>
+ // that the store must be storing the constant value
(else the memory<br>
+ // wouldn't be constant), and this must be a noop.<br>
+ if (AA->pointsToConstantMemory(MI->getDest()))
{<br>
+ // Set the size of the copy to 0, it will be
deleted on the next iteration.<br>
+
MI->setLength(Constant::getNullValue(MI->getLength()->getType()));<br>
+ return MI;<br>
+ }<br>
+<br>
// Extract the length and alignment and fill if they
are constant.<br>
ConstantInt *LenC =
dyn_cast<ConstantInt>(MI->getLength());<br>
ConstantInt *FillC =
dyn_cast<ConstantInt>(MI->getValue());<br>
<br>
Modified:
llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
---
llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
(original)<br>
+++
llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
Mon Apr 22 13:28:19 2019<br>
@@ -1438,6 +1438,12 @@ Instruction
*InstCombiner::visitStoreIns<br>
}<br>
}<br>
<br>
+ // If we have a store to a location which is known
constant, we can conclude<br>
+ // that the store must be storing the constant value
(else the memory<br>
+ // wouldn't be constant), and this must be a noop.<br>
+ if (AA->pointsToConstantMemory(Ptr))<br>
+ return eraseInstFromFunction(SI);<br>
+<br>
// Do really simple DSE, to catch cases where there
are several consecutive<br>
// stores to the same location, separated by a few
arithmetic operations. This<br>
// situation often occurs with bitfield accesses.<br>
<br>
Modified:
llvm/trunk/test/Transforms/InstCombine/memcpy.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memcpy.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memcpy.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/memcpy.ll
(original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/memcpy.ll Mon
Apr 22 13:28:19 2019<br>
@@ -40,7 +40,6 @@ define void @test3(i8* %d, i8* %s) {<br>
<br>
define void @memcpy_to_constant(i8* %src) {<br>
; CHECK-LABEL: @memcpy_to_constant(<br>
-; CHECK-NEXT: call void
@llvm.memcpy.p0i8.p0i8.i32(i8* align 4 bitcast (i128*
@UnknownConstant to i8*), i8* align 1 [[SRC:%.*]], i32
16, i1 false)<br>
; CHECK-NEXT: ret void<br>
;<br>
%dest = bitcast i128* @UnknownConstant to i8*<br>
<br>
Modified:
llvm/trunk/test/Transforms/InstCombine/memmove.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memmove.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memmove.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/memmove.ll
(original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/memmove.ll
Mon Apr 22 13:28:19 2019<br>
@@ -59,7 +59,6 @@ define void @test4(i8* %a) {<br>
<br>
define void @memmove_to_constant(i8* %src) {<br>
; CHECK-LABEL: @memmove_to_constant(<br>
-; CHECK-NEXT: call void
@llvm.memmove.p0i8.p0i8.i32(i8* align 4 bitcast (i128*
@UnknownConstant to i8*), i8* align 1 [[SRC:%.*]], i32
16, i1 false)<br>
; CHECK-NEXT: ret void<br>
;<br>
%dest = bitcast i128* @UnknownConstant to i8*<br>
<br>
Modified:
llvm/trunk/test/Transforms/InstCombine/memset.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memset.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memset.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/memset.ll
(original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/memset.ll Mon
Apr 22 13:28:19 2019<br>
@@ -26,7 +26,6 @@ define i32 @test([1024 x i8]* %target)
{<br>
<br>
define void @memset_to_constant() {<br>
; CHECK-LABEL: @memset_to_constant(<br>
-; CHECK-NEXT: call void @llvm.memset.p0i8.i32(i8*
align 4 bitcast (i128* @Unknown to i8*), i8 0, i32 16,
i1 false)<br>
; CHECK-NEXT: ret void<br>
;<br>
%p = bitcast i128* @Unknown to i8*<br>
<br>
Modified:
llvm/trunk/test/Transforms/InstCombine/store.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/store.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/store.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/store.ll
(original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/store.ll Mon
Apr 22 13:28:19 2019<br>
@@ -295,7 +295,6 @@ define void @write_back7(i32* %p) {<br>
<br>
define void @store_to_constant() {<br>
; CHECK-LABEL: @store_to_constant(<br>
-; CHECK-NEXT: store i32 0, i32* @Unknown, align 4<br>
; CHECK-NEXT: ret void<br>
;<br>
store i32 0, i32* @Unknown<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><u></u><u></u></p>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote></div></div>