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

[Török Edwin]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbginfo-utils.patch
Type: text/x-diff
Size: 4175 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20081215/769c3878/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/769c3878/attachment-0001.patch>