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

[Evan Cheng]

On Dec 15, 2008, at 11:22 AM, 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.

So you are setting DIDescriptor::GV to null? If so, please don't use  
the same name for the parameter. It's confusing.


>
>
>>
>>
>>> 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)

I think it's better to write something like
bool printed = DSI != 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?

Yes, please. Thanks.

Evan

>
>
> Best regards,
> --Edwin
> <dbginfo- 
> utils 
> .patch><pass.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits