<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.im
{mso-style-name:im;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Huh. There are strange interactions here, which makes me even more nervous about testing fewer cases.<o:p></o:p></span></p>
<p class="MsoNormal" style="text-indent:.5in"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">As it happens, the test as written did not exercise all 3 modified paths. Because 'struct S2' had all its members marked with nodebug,
none of the static-member references caused debug info for the class as a whole to be attempted. I needed to add a variable of type S2 (without 'nodebug') in order to exercise that path. Once that was done, then the modification to CollectRecordFields became
necessary.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> Even in an unmodified compiler, "static_const_member" never shows up as a DIGlobalVariable, although the other cases all do. So, testing only
for DIGlobalVariable wouldn't be sufficient to show that it gets suppressed by 'nodebug'.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> "static_member" shows up unless we have modified both EmitGlobalVariable(VarDecl case) and CollectRecordFields, given that the test actually
tries to emit debug info for S2 as a whole.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">So, the genuinely most-minimal did-this-change-do-anything test would need only "static_member" and "const_global_int_def" to show that each path had some effect.
This approach does not fill me with warm fuzzies, or the feeling that future changes in this area will not break the intended behavior. What do you think?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">--paulr<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></a></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [mailto:dblaikie@gmail.com]
<br>
<b>Sent:</b> Wednesday, April 27, 2016 3:43 PM<br>
<b>To:</b> reviews+D19567+public+1ee0c82c0824bedd@reviews.llvm.org; Robinson, Paul<br>
<b>Cc:</b> Aaron Ballman; Adrian Prantl; cfe-commits<br>
<b>Subject:</b> Re: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Wed, Apr 27, 2016 at 3:24 PM, Paul Robinson via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<o:p></o:p></p>
<p class="MsoNormal">probinson added a comment.<br>
<br>
In <a href="http://reviews.llvm.org/D19567#413997" target="_blank">http://reviews.llvm.org/D19567#413997</a>, @dblaikie wrote:<br>
<br>
> For 3 code paths (that seem fairly independent from one another) I'd only really expect to see 3 variables in the test - one to exercise each codepath. What's the reason for the larger set of test cases?<br>
<br>
<br>
I'm not interested in testing code-paths, I'm interested in testing a feature, and relying on the fact that (at the moment) only 3 code paths are involved is not really robust. Granted there is some duplication, and I took that out (r267804), but there are
more than 3 relevant cases from an end-user-feature perspective.<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">We generally have to assume /something/ about the implementation to constrain testing. eg: we don't assume that putting a variable inside a namespace would make a difference here so we don't go & test all these case inside a namespace as
well as in the global namespace. So I'm not sure why we wouldn't go further down that path, knowing that there are only a small number of ways global variable definitions can impact debug info generation.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal"><br>
> Then it might be simpler just to include 6 variables, one of each kind with the attribute, one of each kind without. & I think you could check this reliably with:<br>
<br>
><br>
<br>
> CHECK: DIGlobalVariable<br>
<br>
> CHECK-NEXT: "name"<br>
<br>
><br>
<br>
> repeated three times with a CHECK-NOT: DIGlobalVariable at the end - that way the CHECK wouldn't skip past an existing variable, and you'd check you got the right ones. I think.<br>
<br>
<br>
If I cared only about DIGlobalVariable that does sound like it would work. Sadly, the static const data member comes out as a DIDerivedType, not a DIGlobalVariable. Also, it's *really* important to us that the DICompositeType for "S1" is not present; that's
actually where we get the major benefit.<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">But that codepath is already tested by any test for a TU that has a static member without a definition, right?<br>
<br>
As for the composite type - this is much like testing in LLVM where we test that a certain inlining occurred when we test the inliner - not the knock-on effect that that has on some other optimization which we've already tested separately.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-bottom:12.0pt">Once you have more than one kind of thing to look for (or -NOT look for) it gets a lot more tedious to show it's all correct in one pass like you are suggesting.<br>
<br>
(Re. getting rid of the types: We have licensees with major template metaprogramming going on, but if they can mark those variables 'nodebug' and consequently suppress the instantiated types from the debug info, they should see a major size win. One licensee
reported that reducing some name from 19 characters to 1 char got them a 5MB reduction. Imagine if they could get rid of all of those types...)<br>
<br>
<br>
<span class="im">Repository:</span><br>
<span class="im"> rL LLVM</span><br>
<br>
<span class="im"><a href="http://reviews.llvm.org/D19567" target="_blank">http://reviews.llvm.org/D19567</a></span><br>
<br>
<br>
<o:p></o:p></p>
<div>
<div>
<p class="MsoNormal">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><o:p></o:p></p>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</div>
</body>
</html>