<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 9, 2020 at 10:24 AM Chris Tetreault <<a href="mailto:ctetreau@quicinc.com">ctetreau@quicinc.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 lang="EN-US">
<div class="gmail-m_-2910383471564863989WordSection1">
<p class="MsoNormal">All,<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"> I guess I stand corrected on my statement that nobody uses Release+no debug info+ asserts. Honestly, I find this strange since Debug builds in Visual Studio work perfectly fine and I use the debugger daily, but I’ll not judge. I have
noticed that RelWithDebInfo is heinously slow on Linux, I recently switched to Release there and I’m dreading the next time when I have to actually debug a Linux specific issue. Maybe I’ll try this config as a “better than nothing” option.</p></div></div></blockquote><div><br></div><div>There are things to improve debug info performance/cost on linux - but, yes, the distribution model is different (no pdb files being built on-the-side during compilation - all the debug info goes in the .o files and gets linked into the final executable usually). Split DWARF helps somewhat, but there's still a lot of duplication that's deferred to the linker to cleanup, due to the lack of the compile time inter-compile communication with pdb generation that deduplicates at that time. (this helps MSVC builds locally, but the model doesn't distribute well - so it's not all upside, FWIW)</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 lang="EN-US"><div class="gmail-m_-2910383471564863989WordSection1"><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">> This seems fairly orthogonal to the rest of this discussion, to me at least - it's an improvement over the existing/common<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"> Possibly. I only mentioned it because, in my opinion, David Truby was proposing a band-aid for a bigger issue. That said, if people are actually using -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=TRUE, then my plan breaks apart.
Regardless, I guess I agree that we need two macros.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Thanks,<u></u><u></u></p>
<p class="MsoNormal"> Christopher Tetreault<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><b>From:</b> David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> <br>
<b>Sent:</b> Thursday, April 9, 2020 10:10 AM<br>
<b>To:</b> Chris Tetreault <<a href="mailto:ctetreau@quicinc.com" target="_blank">ctetreau@quicinc.com</a>><br>
<b>Cc:</b> David Truby <<a href="mailto:David.Truby@arm.com" target="_blank">David.Truby@arm.com</a>>; <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> [EXT] Re: [llvm-dev] [RFC] Usage of NDEBUG as a guard for non-assert debug code<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">On Thu, Apr 9, 2020 at 9:59 AM Chris Tetreault via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<u></u><u></u></p>
</div>
<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:5pt 0in 5pt 4.8pt">
<div>
<div>
<p class="MsoNormal">David,<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"> In my opinion, NDEBUG is one of those gross old C things that everybody complains about. It’s called “Not Debug”, but really it means “Assert Disabled”. I think one could be
forgiven for actually using it as a heuristic of whether or not a build is a debug build, especially since no other options are provided. I appreciate your desire, but I think it’d be unfortunate if the build system grew yet another flag to control debugness.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"> As far as I can tell, as it currently works, LLVM_ENABLE_ASSERTIONS just makes sure that NDEBUG is not defined, even in release builds. So if I do -DCMAKE_BUILD_TYPE=Release
-DLLVM_ENABLE_ASSERTIONS=TRUE, I’ll get an optimized build with no debug symbols but with asserts enabled, which in my mind isn’t a terribly useful thing to have.<u></u><u></u></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><br>
FWIW, I believe quite a few people use that mode & don't use debuggers much - faster link/compile times, etc.<br>
<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:5pt 0in 5pt 4.8pt">
<div>
<div>
<p class="MsoNormal">Furthermore, none of this works on Visual Studio because it has a UI menu to control the build type. I personally would be very disappointed to see Visual Studio’s build type dropdown
break.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"> Since we C’s assert, it is intrinsically tied to NDEBUG. What we need is proper custom asserts. In codebases I’ve seen in my travels that have this it usually looks like:<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">// If asserts are enabled, evaluate and assert that expr is truthy. If it is not, complain with msg.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">LLVM_ASSERT(expr, msg)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">// If asserts are enabled, evaluate and assert that expr is truthy. If it is not, complain with msg.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">// If asserts are disabled, evaluate expr, do not assert.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">// either way, return expr</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">LLVM_VERIFY(expr, msg)</span><u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"> The first one is useful as a traditional assert. The second one is useful if you are calling a function, and want to assert that it succeeds, but still need it to be evaluated
in release builds:<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">auto *Foo = LLVM_VERIFY(ReturnsAPointerThatShouldNeverActuallyBeNull(), “this should never return null”);</span><u></u><u></u></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">This seems fairly orthogonal to the rest of this discussion, to me at least - it's an improvement over the existing/common:<br>
<br>
auto *Foo = ReturnsAPointer...;<br>
assert(Foo && "this should never be null");<br>
<br>
But not a fundamentally new thing, etc. (& could be proposed independent of renaming 'assert' to LLVM_ASSERT)<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <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:5pt 0in 5pt 4.8pt">
<div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"> If we have custom asserts, then we can have custom assert guard macros:<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">// true if this is any sort of debug build</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">LLVM_DEBUG_BUILD</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New""> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">// true if asserts are turned on (Debug build on Windows,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">// Debug build or -DLLVM_ASSERTIONS_ENABLED=TRUE on other platforms)</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:9pt;font-family:"Courier New"">LLVM_ASSERTS_ENABLED</span><u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"> These flags could be derived from just CMAKE_BUILD_TYPE, and LLVM_ENABLE_ASSERTIONS can go away (assuming we agree that an asserting build with optimizations and no debug info
is worse than useless).<u></u><u></u></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I don't think people would agree to that - I believe at least a few regular LLVM developers use that mode regularly.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <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:5pt 0in 5pt 4.8pt">
<div>
<div>
<p class="MsoNormal">Custom asserts also have the advantage of having a proper message parameter and not needing to rely on the truthiness of string literals. Obviously this is a much more invasive
change than what you are proposing, but in my opinion it’s the correct thing to do.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">Thanks,<u></u><u></u></p>
<p class="MsoNormal"> Christopher Tetreault<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<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-dev <<a href="mailto:llvm-dev-bounces@lists.llvm.org" target="_blank">llvm-dev-bounces@lists.llvm.org</a>>
<b>On Behalf Of </b>David Truby via llvm-dev<br>
<b>Sent:</b> Thursday, April 9, 2020 7:26 AM<br>
<b>To:</b> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> [EXT] [llvm-dev] [RFC] Usage of NDEBUG as a guard for non-assert debug code<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">Hi all,</span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black"> </span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">During discussions about assertions in the Flang project, we noticed that there are a lot of cases in LLVM that #ifndef NDEBUG is used
as a guard for non-assert code that we want enabled in debug builds. </span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">This works fine on its own, however it affects the behaviour of LLVM_ENABLE_ASSERTIONS; since NDEBUG controls whether assertions are
enabled or not, a lot of debug code gets enabled in addition to asserts if you specify this flag. This goes contrary to the name of the flag I believe also its intention. Specifically in Flang we have a case where someone wants to ship a build with assertions
enabled, but doesn't want to drag in all the extra things that are controlled by NDEBUG in LLVM.</span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black"> </span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">In my opinion we ideally want LLVM_ENABLE_ASSERTIONS to _only_ enable assertions and do nothing else. I don't think this is possible without
changing the use of NDEBUG elsewhere as NDEBUG controls whether assert is enabled.
</span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">I propose we should be using another macro (something like LLVM_DEBUG_CHECKS ?) that is enabled in Debug builds, and possibly controlled
by another cmake flag (LLVM_ENABLE_DEBUG_CHECKS ?) for code that we want enabled for debugging but not in releases. This would allow LLVM_ENABLE_ASSERTIONS to do what it says on the tin and actually enable assertions only.</span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black"> </span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">Does anyone else have any thoughts on this?</span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black"> </span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">Thanks</span><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt;color:black">David Truby</span><u></u><u></u></p>
</div>
</div>
</div>
<p class="MsoNormal">_______________________________________________<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" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><u></u><u></u></p>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote></div></div>