[llvm-commits] [PATCH] Debug info utilities and printer pass

[Török Edwin]

On 2008-12-15 21:22, Török Edwin wrote:
> On 2008-12-15 20:37, Evan Cheng wrote:
>   
>> I don't quite understand this:
>>
>> +// needed by DIVariable::getType()
>> +DIType::DIType(GlobalVariable *GV) : DIDescriptor(GV)
>> +{
>> +  if (GV) {
>> +    unsigned tag = getTag();
>> +    if (tag != dwarf::DW_TAG_base_type && ! 
>> DIDerivedType::isDerivedType(tag) &&
>> +        !DICompositeType::isCompositeType(tag))
>> +      GV = 0;
>> +  }
>> +}
>>
>> What exactly is it trying to accomplish by changing the parameter GV  
>> to 0?
>>   
>>     
>
> There is a comment at the top of the file explaining
> // If this is non-null, check to see if the Tag matches.  If not, set to
> null.
>
> That comment is not duplicated in other places that set GV to 0, so I
> didn't duplicate it either.
>
> If GV is NULL, all the getters in DIDescriptor return a default object,
> instead of trying to read it.
>
> I think it is a way of saying: this object shouldn't have been
> constructed, but it is too late now, and we don't want to throw
> exceptions either,
> so just return default constructed objects from now on.
>
>   
>>   
>>     
>>> 02-dbginfo-utils.patch:
>>> - in my previous patch this was include/llvm/Support/DebugInfo.h, it
>>> introduces:
>>> - findStopPoint
>>> - findBBStopPoint
>>> - findDbgDeclare
>>>     
>>>       
>> +      // a BB consisting only of a terminator, can't have a stoppoint
>>
>> Please start with a capital 'A' and don't forget periods.
>>
>> +      // this BB didn't have a stoppoint, if there is only one
>> +      // predecessor, look for a stoppoint there
>> +      // we could use getIDom(), but that would require dominator info
>>
>> Here as well.
>>
>> +  const DbgDeclareInst *findDbgDeclare(const Value *V, bool stripCasts)
>> +  {
>> +    if (stripCasts) {
>> +      V = V->stripPointerCasts();
>> +      // look for the bitcast
>> +      for (Value::use_const_iterator I = V->use_begin(), E =V- 
>>  >use_end(); I != E; ++I) {
>>
>> 80 column?
>>
>>   
>>     
>
> Thanks, fixed.
>
>   
>>> 03-pass.patch:
>>> - the -print-dbginfo pass from my previous email, now it uses
>>> llvm/Analysis/DebugInfo.h
>>> - caveats: derived types are printed as "", no testcase (should I  
>>> add one?)
>>>
>>>     
>>>       
>> +    // a dbgstoppoint's information is valid until we encounter a new  
>> one
>> +    const DbgStopPointInst *LastDSP = DSI;
>> +    bool printed = !!DSI;
>>
>> Why the "!!"?
>>   
>>     
>
> It is a nice way to transform anything into a bool, I got used to it due
> to writing C macros like:
> #define UNLIKELY(cond) __builtin_expect(!!(cond), 0)
>
>   
>> +        } else if (const DbgFuncStartInst *FS =  
>> dyn_cast<DbgFuncStartInst>(i)) {
>> +
>> +          printFuncStart(FS);
>> +
>> +        }
>>
>> Why the blank lines?
>>   
>>     
>
> Initially printFuncStart wasn't a function, but all the code was there.
> I've removed these lines now.
>
> As for the blank line after this one:
> if ((DSI = dyn_cast<DbgStopPointInst>(i))) {
>
> It was to make the code more readable.
>
> Thanks for the comments, do you see any other major problems, or can I
> commit these patches?
>
> Best regards,
> --Edwin
>   


Sorry, here is the  correct version, the previous patch had a bug (Inst
instead of I).

Best regards,
--Edwin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbginfo-utils.patch
Type: text/x-diff
Size: 4172 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20081215/a468e040/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pass.patch
Type: text/x-diff
Size: 5934 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20081215/a468e040/attachment-0001.patch>