<div dir="ltr">David, thanks for the feedback.<div><br></div><div>The bug fix is integrally tied to the new code, though it was really only by accident - The entire goal was to avoid adding another special-case of "if platform X then assume darwin2" which as can be seen from the deleted code, required twice mentioning every special case.</div><div><br></div><div>One of my comments in the patch lists the test files whose changes are actually meaningful, as opposed to test that were forced against their will to go along for the ride. The test with explicitly changed expectation is mentioned - it's "cl-options.c" but I'll explain here too:</div><div><br></div><div>A test specified "/z7" and "-gdwarf" in that order.  "/z7" means line-tables-only.  But "-gdwarf" implies normal ("limited") debug info.</div><div>The "/z7" option was properly translated into "-gline-tables-only" but did not check whether it occurred earlier or later than any other 'g' option. (It is not a g-group flag).</div><div>So in this one case, and only this case, you have both a line-tables-only flag and a more-than-lines-tables-only flag getting through to CompilerInvocation. Then when you look at what CompilerInvocation did, it asked "Args.hasArg(OPT_gline_tables_only)" as the first check, and if so, did not care about presence of other 'g' flags. So the leftmost arg "won". It clearly assumed that at most one of any of the conflicting g flags would have been picked by the Driver.</div><div><br></div><div>The new code's 3-valued argument can't really get this wrong in the same way. Usually I don't like to change the expectation of one test in a big suite of tests like this, but to avoid changing the test's expectation would require temporarily injecting a bug-for-bug compatible behavior saying that "/z7" is always stronger than '-g'.  But really that's a bit silly, don't you think?</div><div><br></div><div> </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 2, 2015 at 6:17 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Oct 1, 2015 at 8:18 AM, Douglas Katzman via cfe-dev <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span style="font-size:12.8px">Hi,</span><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">I have a patch up on Phabricator which regularizes some 'g' arguments in the tool frontends by trusting the clang Driver to handle toolchain-specific choices about the option family, such as when the toolchain defaults to Dwarf 2 vs Dwarf 4 and/or when -fstandalone-debug should be assumed.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">The internal option name for setting DebugInfoKind is "-di-kind={full|limited|line-tables}" which directly sets the codegen opts to the enum value whose name is as given.</div><div style="font-size:12.8px">Similarly there's a dwarf-version option.  So the '-g' group options will be stripped out by the driver and not passed along to cc1 or cc1as.</div><div style="font-size:12.8px">Specifically, the '-gdwarf', '-gline-tables-only', and '-g' options will be rejected if seen in {cc1,cc1as} invocations that were hand-crafted in a lot of regression tests.</div></div></blockquote><div><br></div></span><div>Yes, cc1/cc1as commmand lines are intentionally hand crafted in tests to limit the scope of a test. Driver tests should test only the driver (just check how the normal command line args are mapped to cc1/cc1as) and other tests should not test the driver.<br><br>But the cc1/cc1as interface is not stable, and up to us to change at will. So it's intentional that these tests are like this, but not an indication that they need to stay that way - any changes there should be totally fine, so long as they produce the same effect overall.<br><br>(this is similar to the boundary between Clang and LLVM being LLVM IR - we can and do change the textual IR and just update tests on both sides of that boundary whenever we do so)</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div style="font-size:12.8px"><span style="font-size:12.8px">I don't think this should be controversial, as the cc1 invocation is not considered a public API, but maybe some folks would prefer that '-g' still have some meaning.</span></div></div></blockquote><div><br></div></span><div>I don't think it's important for cc1 to have a -g option.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div style="font-size:12.8px"><span style="font-size:12.8px">So I'd like to ask whether that's the case, as well as invite a little bit of "bikeshed" discussion as to whether the option name I've introduced makes sense.</span><br></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px"><span style="font-size:12.8px">The names "limited" and "full" have slightly strange connotations even though they have the right semantics. </span><br></div><div style="font-size:12.8px">"limited" is the normal level of debugging, whereas "full" means "-fstandalone" because the debugger is unable to cope with "limited".<br></div><div style="font-size:12.8px">So it would also be totally reasonable to name these choices {standalone,normal,line-tables} which unfortunately don't coincide with names of DebugInfoKind.</div></div></blockquote><div><br></div></span><div>I'm happy with a smaller taxonomy and for cc1 to match the source code (rather than the command line) more closely... probably. (arguably we could change the implementation to more closely match the command line, if it's a good taxonomy - I dunno)<br><br>The "limited" terminology came from a narrower version of -fstandalone from a long time ago (-flimit-debug-info was the default and you could opt-out with -fno-limit-debug-info). I improved and then expanded on that until it broke a bunch of stuff for some debuggers. So we wanted to describe better the principles by which users could understand when they might need to use this feature. ("standalone" being "if I'm building this object file with debug info and I don't want the compiler to assume other parts of the program will be built with debug info": ie: let the debug info for this object stand on its own/standalone)<br><br>(in case any of that history is helpful to you)</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div style="font-size:12.8px">I chose the names that matched, but I find them unappealing.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">So for what it's worth, could some folks maybe express an opinion, and/or take a crack at reviewing this?</div><div style="font-size:12.8px"> <a href="http://reviews.llvm.org/D13221" target="_blank">http://reviews.llvm.org/D13221</a></div></div></blockquote><div><br></div></span><div>Taking a quick glance - is it possible to separate moving the info from frontend to driver from the bug fixes that enables? Or are they intrinsically tied?<br><br>It'd just be a bit easier to review if they were separate - otherwise you might need to point us to where the bug fix test improvements/changes have been made, amongst all the other changes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><div dir="ltr"><div style="font-size:12.8px"><span style="font-size:12.8px">Also I'll note one pro and one con for keeping '-g' as an option to {cc1, cc1as}.</span><br></div><div style="font-size:12.8px">Pro: some of the tests that were affected in the above change could remain untouched.</div><div style="font-size:12.8px">Con: it's better to visibly fail than to silently accept an option whose meaning differs, which is to say, if people expected that the CompilerInvocation would automatically use Dwarf 2 on Darwin when you give '-g' in the cc1 invocation, that is most definitely no longer true.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">Thanks and regards,</div><div style="font-size:12.8px">Doug</div></div>
<br></span>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div></div>
</blockquote></div><br></div>