<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Nov 22, 2013 at 9:54 AM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@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 class="HOEnZb"><div class="h5">On Fri, Nov 22, 2013 at 9:08 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Thu, Nov 21, 2013 at 4:17 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>><br>
>> On Thu, Nov 21, 2013 at 12:01 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> On Thu, Nov 21, 2013 at 11:45 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>>> On Thu, Nov 21, 2013 at 11:26 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>> On Thu, Nov 21, 2013 at 11:06 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>>>>> wrote:<br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> On Thu, Nov 21, 2013 at 10:57 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>>>>>> wrote:<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> On Thu, Nov 21, 2013 at 10:38 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>>>>>>> wrote:<br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>> On Tue, Nov 19, 2013 at 8:47 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>>>>>>>> wrote:<br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>> On Tue, Nov 19, 2013 at 5:26 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>>>>>>>>> wrote:<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> On Mon, Nov 18, 2013 at 11:00 AM, Chandler Carruth<br>
>>>>>>>>>> <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
>>>>>>>>>>><br>
>>>>>>>>>>><br>
>>>>>>>>>>> On Mon, Nov 18, 2013 at 10:55 AM, David Blaikie<br>
>>>>>>>>>>> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>>>>>>>>>>><br>
>>>>>>>>>>>> It depends a bit, also, on what kind of guarantees we need to<br>
>>>>>>>>>>>> offer. If the guarantee when reading IR from disk is "will not crash" then<br>
>>>>>>>>>>>> there's nothing for it but to run full debug info verification.<br>
>>>>>>>>>>>><br>
>>>>>>>>>>>> On the other hand, if we can assume that some specific metadata<br>
>>>>>>>>>>>> implies the correctness of some other metadata, then all we need to do is<br>
>>>>>>>>>>>> check a known debug info version number. If it's the current number, do<br>
>>>>>>>>>>>> nothing (assume the rest of the debug info is up to date and well formed),<br>
>>>>>>>>>>>> otherwise do all the work to find the debug info and dump it (no need to<br>
>>>>>>>>>>>> verify it - we just have to do the work to find it, so we don't go following<br>
>>>>>>>>>>>> bad links later on - that should be as easy as dropping the <a href="http://llvm.dbg.cu" target="_blank">llvm.dbg.cu</a><br>
>>>>>>>>>>>> named node and removing all debug intrinsics and the instruction metadata<br>
>>>>>>>>>>>> line references). But this latter scheme isn't robust against arbitrary<br>
>>>>>>>>>>>> metadata (that could, coincidentally, have the right version number and<br>
>>>>>>>>>>>> arbitrary metadata that breaks all our debug info metadata assumptions)<br>
>>>>>>>>>>>><br>
>>>>>>>>>>>> If the latter is sufficient for everyone's needs/principles,<br>
>>>>>>>>>>>> great.<br>
>>>>>>>>>>><br>
>>>>>>>>>>><br>
>>>>>>>>>>> This makes sense to me, but I see Eric's fundamental concern with<br>
>>>>>>>>>>> upgrading test cases.<br>
>>>>>>>>>>><br>
>>>>>>>>>>><br>
>>>>>>>>>>> One other possible idea: we could artificially force coarseness<br>
>>>>>>>>>>> on metadata while things are still iterating rapidly by just having a<br>
>>>>>>>>>>> version number that rotates every "release". (Even non-open-source-releases<br>
>>>>>>>>>>> could be handled by letting anyone, any time they need to rev it, update<br>
>>>>>>>>>>> what the current version is and update every test case to reference that<br>
>>>>>>>>>>> version.) If the version is nicely factored, it should at least be an<br>
>>>>>>>>>>> obvious diff if still huge.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> Adding a debug info version number and ignoring the debug info<br>
>>>>>>>>>> MDNodes when version does not match makes sense. And changing the version<br>
>>>>>>>>>> number with every release sounds reasonable.<br>
>>>>>>>>>><br>
>>>>>>>>>> One implementation is<br>
>>>>>>>>>> 1> completely get rid of version number in the tag<br>
>>>>>>>>>> LLVMDebugVersion is only used in two places and we should be<br>
>>>>>>>>>> able to remove it.<br>
>>>>>>>>>> include/llvm/DebugInfo.h: return getUnsignedField(0) &<br>
>>>>>>>>>> ~LLVMDebugVersionMask;<br>
>>>>>>>>>> lib/IR/DIBuilder.cpp: assert((Tag & LLVMDebugVersionMask) ==<br>
>>>>>>>>>> 0 &&<br>
>>>>>>>>>> lib/IR/DIBuilder.cpp: return<br>
>>>>>>>>>> ConstantInt::get(Type::getInt32Ty(VMContext), Tag | LLVMDebugVersion);<br>
>>>>>>>>>> I remember there was something that blocks David from completely<br>
>>>>>>>>>> getting rid of version number, but it seems the blocker is gone now.<br>
>>>>>>>>>> This can be done separately as well.<br>
>>>>>>>>>><br>
>>>>>>>>>> 2> introduce a debug info version in module flags similar to<br>
>>>>>>>>>> "dwarf version" module flag<br>
>>>>>>>>>> LTO can correctly handle the module flag during linking and<br>
>>>>>>>>>> we only need to change one line in the debug info testing cases when we<br>
>>>>>>>>>> update debug info version number.<br>
>>>>>>>>>> One issue is that we don't have a mode that emits a warning<br>
>>>>>>>>>> and outputs the largest value (we have a mode that emits a warning and<br>
>>>>>>>>>> outputs the value from the first module, but we can definitely add another<br>
>>>>>>>>>> mode if necessary)<br>
>>>>>>>>>><br>
>>>>>>>>>> 3> when to drop the debug info MDNodes?<br>
>>>>>>>>>> We could do this in the bit code loader but it seems a<br>
>>>>>>>>>> violation of layers. One possibility is to run a module pass right after<br>
>>>>>>>>>> loading the bit code, to remove debug intrinsics and debug tags on<br>
>>>>>>>>>> instructions.<br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>> We have an existing pass StripSymbols that can strip debug info in<br>
>>>>>>>>> the module.<br>
>>>>>>>>> So it is just a matter of adding that pass after bit code loader if<br>
>>>>>>>>> the debug info version number does not match.<br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>> Can I assume no objection to the approach? :)<br>
>>>>>>>> Bill, are you okay with adding another mode to module flags? i.e.<br>
>>>>>>>> emits a warning and outputs the largest value.<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> How will this work as a module flag, though? If the flag gets merged<br>
>>>>>>> and maximized, how will we know which compile units have out of date debug<br>
>>>>>>> info metadata and drop them?<br>
>>>>>><br>
>>>>>><br>
>>>>>> The dropping part comes from item 3 above: adding StripSymbols pass<br>
>>>>>> right after bit code loader if the debug info version number does not match.<br>
>>>>><br>
>>>>><br>
>>>>> (what Eric said, but also)<br>
>>>><br>
>>>><br>
>>>>><br>
>>>>><br>
>>>>> I'm just not really understanding how you choose which things to strip<br>
>>>>> in (3). If you've already merged IR modules together and the debug info<br>
>>>>> version is bumped to the maximum - if we had an old and a new module, the<br>
>>>>> resulting version would be 'new' and we wouldn't know we still had some old<br>
>>>>> data in there we needed to remove, would we? (and even if we did, we<br>
>>>>> wouldn't know which of the compile units we needed to drop and which<br>
>>>>> intrinsics we needed to strip, etc - since some was new and good and some<br>
>>>>> was old and busted)<br>
>>>><br>
>>>><br>
>>>> To David,<br>
>>>><br>
>>>> I was saying that running StripSymbols pass right after loading a bit<br>
>>>> code file (a source module), then the merging part will come afterwards when<br>
>>>> linking the source module in.<br>
>>><br>
>>><br>
>>> OK - but then we'd need another special case for non-LTO, when someone's<br>
>>> just passed a bitcode file to Clang, for example - yes?<br>
>>><br>
>>> By building it into the loader or as close to that as possible (so that<br>
>>> any LLVM code loading a module never sees out of date debug info metadata)<br>
>>> we ensure we only have to change one place, not every client of bitcode<br>
>>> loading.<br>
>><br>
>><br>
>> We can drop the debug info in the auto upgrading path similar to how we<br>
>> upgrade TBAA tags.<br>
>> I am going to move StripDebugInfo from StripSymbols.cpp to DebugInfo.cpp<br>
>> so the implementation can be shared between StripSymbols and AutoUpgrade.<br>
>> After that, a sample patch looks like:<br>
>> Index: include/llvm/AutoUpgrade.h<br>
>> ===================================================================<br>
>> --- include/llvm/AutoUpgrade.h (revision 195264)<br>
>> +++ include/llvm/AutoUpgrade.h (working copy)<br>
>> @@ -57,6 +57,10 @@<br>
>> /// with different address spaces: the instruction is replaced by a<br>
>> pair<br>
>> /// ptrtoint+inttoptr.<br>
>> Value *UpgradeBitCastExpr(unsigned Opc, Constant *C, Type *DestTy);<br>
>> +<br>
>> + /// Check the debug info version number, if it is out-dated, drop the<br>
>> debug<br>
>> + /// info.<br>
>> + bool UpgradeDebugInfo(Module &M);<br>
>> } // End llvm namespace<br>
>><br>
>> #endif<br>
>> Index: lib/AsmParser/LLParser.cpp<br>
>> ===================================================================<br>
>> --- lib/AsmParser/LLParser.cpp (revision 195264)<br>
>> +++ lib/AsmParser/LLParser.cpp (working copy)<br>
>> @@ -182,6 +182,8 @@<br>
>> for (Module::iterator FI = M->begin(), FE = M->end(); FI != FE; )<br>
>> UpgradeCallsToIntrinsic(FI++); // must be post-increment, as we<br>
>> remove<br>
>><br>
>> + UpgradeDebugInfo(*M);<br>
>> +<br>
>> return false;<br>
>> }<br>
>><br>
>> Index: lib/Bitcode/Reader/BitcodeReader.cpp<br>
>> ===================================================================<br>
>> --- lib/Bitcode/Reader/BitcodeReader.cpp (revision 195264)<br>
>> +++ lib/Bitcode/Reader/BitcodeReader.cpp (working copy)<br>
>> @@ -3152,6 +3152,7 @@<br>
>> for (unsigned I = 0, E = InstsWithTBAATag.size(); I < E; I++)<br>
>> UpgradeInstWithTBAATag(InstsWithTBAATag[I]);<br>
>><br>
>> + UpgradeDebugInfo(*M);<br>
>> return error_code::success();<br>
>> }<br>
>><br>
>> Index: lib/IR/AutoUpgrade.cpp<br>
>> ===================================================================<br>
>> --- lib/IR/AutoUpgrade.cpp (revision 195264)<br>
>> +++ lib/IR/AutoUpgrade.cpp (working copy)<br>
>> @@ -12,6 +12,7 @@<br>
>><br>
>> //===----------------------------------------------------------------------===//<br>
>><br>
>> #include "llvm/AutoUpgrade.h"<br>
>> +#include "llvm/DebugInfo.h"<br>
>> #include "llvm/IR/Constants.h"<br>
>> #include "llvm/IR/Function.h"<br>
>> #include "llvm/IR/IRBuilder.h"<br>
>> @@ -489,3 +490,11 @@<br>
>><br>
>> return 0;<br>
>> }<br>
>> +<br>
>> +bool llvm::UpgradeDebugInfo(Module &M) {<br>
>> + if (getDebugInfoVersionFromModule(M) == LLVMDebugVersion)<br>
>> + return false;<br>
>> +<br>
>> + StripDebugInfo(M);<br>
>> + return true;<br>
>> +}<br>
>><br>
>> Of course, testing cases will be added when I actually submit the patch.<br>
>><br>
>>><br>
>>>><br>
>>>><br>
>>>> To Eric,<br>
>>>> < You'll want to do it at loading time and not worry about merging at<br>
>>>> < all. I think this will need to go just after reading the module and<br>
>>>> < check the version against the hard coded version in the bit code<br>
>>>> < reader/somewhere. I think we should do it there rather than as a later<br>
>>>> < pass as we really don't want to do anything to the metadata other than<br>
>>>> < strip it.<br>
>>>><br>
>>>> I think I was saying the same thing: right after bit code loader :)<br>
>>>> Eric, are you suggesting to strip the debug info inside the loader?<br>
>>>><br>
>>>> The merging part is mostly about where to store the debug info version<br>
>>>> number in the bit code file.<br>
>>>> If we store it as a module flag, we need to specify a mode for the<br>
>>>> module flag so the linker knows how to merge the module flag.<br>
>>><br>
>>><br>
>>> Except we'd never actually need to merge the flag - since we would drop<br>
>>> it from any module that didn't have the latest, right? I'm not sure this<br>
>>> would need to be a module flag, then, since there would be no need to define<br>
>>> merging behavior. Is there an advantage to using a module flag over just<br>
>>> putting it on the compile unit metadata node somewhere?<br>
>><br>
>><br>
>> Agreed. So we have two choices here, use a module flag and remove the<br>
>> module flag when stripping the debug info, so we will never merge modules<br>
>> with different debug info versions. (no new mode is required)<br>
>><br>
>> The other choice is putting it on the compile unit MDNode as a field.<br>
>> There is no reliable way of checking if an old bc file has a version<br>
>> number, because it requires parsing the CU MDNodes in the old bc files.<br>
>> Also if we are not going to support multiple debug info versions in a<br>
>> single compiler, there is no point of putting a version number in each CU<br>
>> MDNode.<br>
><br>
><br>
> Fair point. Any useful tradeoffs to consider between a single named MDNode<br>
> and a module flag?<br>
><br>
<br>
</div></div>Probably simplicity at the moment. A module flag is pretty easy to<br>
read/parse/etc at load time versus having to put it in the metadata<br>
itself.</blockquote><div><br></div><div>If the module flag is easier - I'm all for it. I wasn't sure if they were schematized more strictly than metadata or anything.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In the metadata itself though we'd have everything self<br>
contained (i.e. just stick it as a field on the compile unit or<br>
something).<br></blockquote><div><br></div><div>I'm with Manman on the "if we never have mismatched versions, paying a per-CU overhead is silly" position, so I'd put it in its own named metadata instead (since necessarily anything other than the loading code would never see CUs with anything other than the current debug version, so it would be entirely redundant) - but sounds like module is simpler, so that's great.<br>
<br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
>><br>
>><br>
>> If we decide to go with the module flag, I can start adding the module<br>
>> flag and update all existing testing cases. Let me know your vote :)<br>
>><br>
>> Manman<br>
>><br>
>>><br>
>>>><br>
>>>> If we decide to save the debug info version number as a module flag,<br>
>>>> then we may need another mode for it.<br>
>>><br>
>>><br>
>>> I would imagine we could strip the flag whenever we strip the debug info,<br>
>>> then we would never merge modules that had differing debug info versions. So<br>
>>> we could use any merging (or none at all) definition, since we'd only ever<br>
>>> merge matching ones.<br>
>>><br>
>>>><br>
>>>><br>
>>>> Thanks,<br>
>>>> Manman<br>
>>>><br>
>>>>><br>
>>>>>><br>
>>>>>> Item 3 is shared between non-LTO and LTO. For LTO, we want the debug<br>
>>>>>> info version number gets merged and maximized.<br>
>>>>>><br>
>>>>>> Thanks<br>
>>>>>> Manman<br>
>>>>>><br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> - David<br>
>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>>> Thanks,<br>
>>>>>>>> Manman<br>
>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>> 4> how to report warning?<br>
>>>>>>>>> Is errs() << "WARNING: " good enough? BTW this is how we emit<br>
>>>>>>>>> warning when linking module flags.<br>
>>>>>>>>><br>
>>>>>>>>> Any objection to the above? Any other suggestion?<br>
>>>>>>>>><br>
>>>>>>>>> Thanks,<br>
>>>>>>>>> Manman<br>
>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> _______________________________________________<br>
>>>>>>>>>> LLVM Developers mailing list<br>
>>>>>>>>>> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>>>>>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>><br>
>>>>>>>><br>
>>>>>>><br>
>>>>>><br>
>>>>><br>
>>>>><br>
>>>><br>
>>><br>
>><br>
><br>
</div></div></blockquote></div><br></div></div>