Patch for missing debug info of type being explicitly casted to

[• jyoti.yalamanchili]

Summary

Hi,
Debug info of types missing in case of explicit casts.

Example :
typedef struct S { int i; } *T;
void foo(void *p) { ((T)(p))->i++; }

No DW_TAG_typedef, DW_TAG_structure_type and DW_TAG_member tags are available in debug text.
This patch fixes the same and produces tags irrespective of whether typedef is used or not.

Thanks!

[dblaikie]

I wonder if there's a more general place we should be handling this? And
what about non-typedefs used in this context?

The test case should use filecheck, not grep.

The comment in emittypedef is a bit obscure.

[• jyoti.yalamanchili]

Thanks for your quick feedback.
I will modify the comment & grep as suggested and also check if it can be moved to CodeGenXX files.
Could you please clarify what you mean by ' non-typedefs used in this context ?'

[dblaikie]

I mean what happens if we use a concrete type in that context, rather than
a ty pedef? Do we see the same bug?

[• jyoti.yalamanchili]

This bug appears only when typedef is used.

[echristo]

Needs changes, thanks for working on this!

llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp

3280 ↗

(On Diff #6326)

Please follow the style guide for formatting. The entire patch.

3283 ↗

(On Diff #6326)

"where"

llvm/tools/clang/lib/CodeGen/CGExprScalar.cpp

281 ↗

(On Diff #6326)

Only == Limited? You probably mean >=.

I'm also not sure this is the right place for this, Dave any thoughts?

llvm/tools/clang/test/CodeGen/Debug-info-typedef.cpp

1 ↗

(On Diff #6326)

Testcase should check the generated metadata, not the assembly. It'll also mean you can just use a single run of the test and use FileCheck.

[• jyoti.yalamanchili]

Hi Eric,
I have implemented the review comments.This issue is specific to explicit cast.
Debug info of type being explicitly casted to was not emitted.
I have added an elaborate testcase to capture the issue which should be resolved with this patch. Verified the regression on 216904 revision.
Could you please review this for me and commit if you deem it okay.

[dblaikie]

In D2498#4, @jyoti.yalamanchili wrote:

This bug appears only when typedef is used.

Doesn't look like it.

Changing the code to cast to (S*) instead of (T) still demonstrates the bug and is still present even with this patch applied.

The problem is more general than typedefs and casts - arguably we should get this (C++11) case too:

struct S { int i; };
void func(S);
void func() {
  func(S{42});
}

but GCC doesn't get this either, so I'm less concerned.

GCC does get the cast case - with or without a typedef. So perhaps we can just deal with casts for now...

The only other thing I can think of is far more invasive/more work - to get this all right, what we really need is a tree of "completeness" - keeping track of any time a type is required to be complete (not just a single bit that says "this has been required to be complete") & then emitting any type for which it was requried to be complete during a function we codegen'd.

This infrastructure would also allow us to do a better job minimizing debug info for the limited debug info optimization where we currently fail:

struct foo { ... };
inline void unused() { foo f; } // 'f' is required to be complete here, but the function is never called
foo *f; // but that required completeness is never relied upon for this TU - a declaration would've sufficed

This hurts us in, for example, Clang's own Sema.h - the giant Sema class has a single non-member inline function right after it that has dereferences a Sema pointer, thus requiring Sema to be complete. From then on, if anything else in the TU including Sema.h requires Sema (evern just a pointer to it it) the debug info for the entire class will be emitted.

[• jyoti.yalamanchili]

Hi David,

In D2498#9, @dblaikie wrote:

In D2498#4, @jyoti.yalamanchili wrote:

This bug appears only when typedef is used.

Doesn't look like it.

This was my previous analysis which was incorrect. This issue occurs whenever explicit casting occurs. The type to which we are explicitly casting is not emitted in debug info.

Changing the code to cast to (S*) instead of (T) still demonstrates the bug and is still present even with this patch applied.

As you mentioned, I verified this for the following code snippet where i am replacing T with struct S* and the result was that DW_TAG_structure_type & DW_TAG_member were emitted but not DW_TAG_typedef because T was not used. I thought this is expected behaviour. Please correct me if i am wrong.

typedef struct S {

int i;

} *T;
#define M(p) ((struct S*)(p))

void foo(void *p) { M(p)->i++; }

The problem is more general than typedefs and casts - arguably we should get this (C++11) case too:

struct S { int i; };
void func(S);
void func() {
  func(S{42});
}

but GCC doesn't get this either, so I'm less concerned.

GCC does get the cast case - with or without a typedef. So perhaps we can just deal with casts for now...

Okay.

The only other thing I can think of is far more invasive/more work - to get this all right, what we really need is a tree of "completeness" - keeping track of any time a type is required to be complete (not just a single bit that says "this has been required to be complete") & then emitting any type for which it was requried to be complete during a function we codegen'd.

Sorry, could you please explain this with an example ?
I could mention this in the testcase as a TODO note if you want so we don't loose track of this improvement scope.

This infrastructure would also allow us to do a better job minimizing debug info for the limited debug info optimization where we currently fail:

struct foo { ... };
inline void unused() { foo f; } // 'f' is required to be complete here, but the function is never called
foo *f; // but that required completeness is never relied upon for this TU - a declaration would've sufficed

This hurts us in, for example, Clang's own Sema.h - the giant Sema class has a single non-member inline function right after it that has dereferences a Sema pointer, thus requiring Sema to be complete. From then on, if anything else in the TU including Sema.h requires Sema (evern just a pointer to it it) the debug info for the entire class will be emitted.

If i understand this correctly, point of worry here is about the increase in debug info size, am i right? But, we certainly need the debug info of any referenced variables isn't ?

[dblaikie]

In D2498#10, @jyoti.yalamanchili wrote:

Hi David,

In D2498#9, @dblaikie wrote:

In D2498#4, @jyoti.yalamanchili wrote:

This bug appears only when typedef is used.

Doesn't look like it.

This was my previous analysis which was incorrect. This issue occurs whenever explicit casting occurs. The type to which we are explicitly casting is not emitted in debug info.

Changing the code to cast to (S*) instead of (T) still demonstrates the bug and is still present even with this patch applied.

As you mentioned, I verified this for the following code snippet where i am replacing T with struct S* and the result was that DW_TAG_structure_type & DW_TAG_member were emitted but not DW_TAG_typedef because T was not used. I thought this is expected behaviour. Please correct me if i am wrong.

Looks like I was confused (perhaps by the old title of this code review which still mentions typedefs - and perhaps by Phab showing me the newer diff, or maybe I was looking in the wrong place somehow and saw the old code which still special-cased for typedefs). Now that I apply your latest patch properly I see the behavior you describe - this fixes the issue regardless of whether a typedef is used.

typedef struct S {

int i;

} *T;
#define M(p) ((struct S*)(p))

void foo(void *p) { M(p)->i++; }

The problem is more general than typedefs and casts - arguably we should get this (C++11) case too:

struct S { int i; };
void func(S);
void func() {
  func(S{42});
}

but GCC doesn't get this either, so I'm less concerned.

GCC does get the cast case - with or without a typedef. So perhaps we can just deal with casts for now...

Okay.

The only other thing I can think of is far more invasive/more work - to get this all right, what we really need is a tree of "completeness" - keeping track of any time a type is required to be complete (not just a single bit that says "this has been required to be complete") & then emitting any type for which it was requried to be complete during a function we codegen'd.

Sorry, could you please explain this with an example ?

struct foo { };
inline void func() { foo f; } // this makes 'f' required to be complete, which means we emit a definition of 'f' if we ever need any reference to 'f'
foo *f; // this causes us to emit 'f'

Without the second line, we just emit a declaration (in the default -fno-standalone-debug mode).

I could mention this in the testcase as a TODO note if you want so we don't loose track of this improvement scope.

That's OK - it's pretty much outside the scope of your change. It wouldn't really belong in this test file.

This infrastructure would also allow us to do a better job minimizing debug info for the limited debug info optimization where we currently fail:

struct foo { ... };
inline void unused() { foo f; } // 'f' is required to be complete here, but the function is never called
foo *f; // but that required completeness is never relied upon for this TU - a declaration would've sufficed

This hurts us in, for example, Clang's own Sema.h - the giant Sema class has a single non-member inline function right after it that has dereferences a Sema pointer, thus requiring Sema to be complete. From then on, if anything else in the TU including Sema.h requires Sema (evern just a pointer to it it) the debug info for the entire class will be emitted.

If i understand this correctly, point of worry here is about the increase in debug info size, am i right?

The -fno-standalone-debug default option is an attempt to minimize debug info size, yes. The issue is that it uses /some/ part of the powers necessary to make our general debug info strategy more reliable and to fix this bug you're addressing and possible others in a more general manner. Though I can't site any particular examples similar to yours (places where a type is used, but no named instance or reference to the type is ever needed)... actually I can:

struct S { };
void *v = new S;

Clang doesn't emit debug info describing 'S' (but then again, neither does GCC). I imagine maybe there's a few other pieces of syntax that allow one to reference a type without ever naming a variable of that type.

But, we certainly need the debug info of any referenced variables isn't ?

Sure - in this case there was no variable, but it's still reasonable to describe the type. The rough heuristic is: Any expression written in executing code (global intializers, functions that are actually codegen'd, etc) should be able to be written and executed in a sufficiently advanced debugger given the information Clang produces.

Any cases where the information is fundamentally not there - are bugs.

llvm/tools/clang/lib/CodeGen/CGExprScalar.cpp

279 ↗

(On Diff #13589)

Roll the DI variable into the if condition and sink the LimitedDebugInfo check into the DI function (at least I think that's how we are/should be doing this - keeping as much of the implementation details down in CGDebugInfo rather than up in the CodeGen callsites)

llvm/tools/clang/test/CodeGen/Debug-info-explicitcast.c

4 ↗

(On Diff #13589)

Could you drop the macro usage and the typedef as they're not needed to demonstrate the fundamental bug here?

[• jyoti.yalamanchili]

Hi David,
I have updated the patch with your review comments.
Sorry about the confusion caused, i should have updated the title and comment fo
r Diff2 before.

[• jyoti.yalamanchili]

Hi David,
I have updated the patch with your review comments.

Sorry about the confusion caused, i should have updated the title and
comment for Diff2 before.

[dblaikie]

llvm/tools/clang/test/CodeGen/Debug-info-explicitcast.c

7 ↗

(On Diff #13832)

There's no need to check the metadata in this level of detail (especially the block of checks at the end of this file) - it makes the tests particularly brittle to metadata schema changes.

Check out some of the other tests to get a sense of the minimal metadata needing to be checked.

Drop the typedef from the test - since that's not the unedrlying issue, and just test the cast to "struct S*" then in the CHECK, something as simple as:

CHECK: ; [ DW_TAG_structure_type ] [S]

should be sufficient to demonstrate that the type was emitted.

As a side note, given this code /in C++/ (coupled with your change):

struct S { };
void sink(S*);
void foo(void* v) {
  sink((S*)v);
}

do we emit the declaration of S? (ie: do we emit S (rather than leaving it out entirely) and do we only emit a declaration (since the full definition of S was never needed)? - arguably we could just avoid emitting it entirely here, since no one ever dereferenced the pointer and we don't need the declaration to describe any other entities... )

[• jyoti.yalamanchili]

Hi David,
Modified the testcase as you suggested.
I have replied inline for your query below.

Thank You!

[dblaikie]

llvm/tools/clang/test/CodeGen/Debug-info-explicitcast.c

13 ↗

(On Diff #13868)

This case isn't so interesting, and isn't related to explicit casts - this case the type S3 will be emitted because we emit the function definition for foo3 and need to describe the type of its parameter (initially it would be built as just a declaration, but then when we see the ->, the type will be required to be complete and we'll upgrade it to a definition)

So just remove this test.

19 ↗

(On Diff #13868)

This test doesn't seem to be necessary either - your change doesn't have anything union-specific. Type building is type building and is fairly well tested elsewhere - redundant tests just make for longer test times and more maintenance when we do need to change something that affects the results of such code.

[• jyoti.yalamanchili]

Hi David,
Yes, you are right. I have modified the testcase.
Thanks a lot for your review and patience.

Regards
Jyoti Allur

[• jyoti.yalamanchili]

llvm/tools/clang/test/CodeGen/Debug-info-explicitcast.c

7 ↗

(On Diff #13832)

Okay, i'll modify the testcase to check minimal metdata and remove typedef cases as you suggested.

For the code above, we do emit a declaration of S with my patch, since v was casted to type S. This behaviour is inline with gcc. Without my patch declaration of S was not emitted.

I did try with this modified code and no declaration of S was emitted.

struct S { };
void sink(void*);
void foo(void* v) {

sink(v);

}

[• jyoti.yalamanchili]

Hi David,
In this diff test file name was modified to follow naming convention of other test files in CodeGen + review comment implements from before. Please also refer to reply inline comment above for a query you had thrown before.

Thanks !

[dblaikie]

Looks good to me. Though one day we might want to generalize the name of this function and call it from a few more places where types can be referenced without otherwise being attached to debug info ( "func(void*); ... func(new T());" comes to mind - for some trivial type T (ie: no ctor, etc)).

[• jyoti.yalamanchili]

This comment was removed by • jyoti.yalamanchili.

[• jyoti.yalamanchili]

Hi David,
I will keep this in mind to generalize this function name change when submitting another patch.
Could you please submit this for me. I do not have commit permission yet.
Thank You.

[dblaikie]

Closed by commit rL218390 (authored by @dblaikie).

cfe/trunk/lib/CodeGen/CGDebugInfo.h

public:
   void EmitGlobalVariable(llvm::GlobalVariable *GV, const VarDecl *Decl);
 
   /// EmitGlobalVariable - Emit global variable's debug info.
   void EmitGlobalVariable(const ValueDecl *VD, llvm::Constant *Init);
 
   /// \brief - Emit C++ using directive.
   void EmitUsingDirective(const UsingDirectiveDecl &UD);
 
+  /// EmitExplicitCastType - Emit the type explicitly casted to.
+  void EmitExplicitCastType(QualType Ty);
+
   /// \brief - Emit C++ using declaration.
   void EmitUsingDecl(const UsingDecl &UD);
 
   /// \brief - Emit C++ namespace alias.
   llvm::DIImportedEntity EmitNamespaceAlias(const NamespaceAliasDecl &NA);
 
   /// getOrCreateRecordType - Emit record type's standalone debug info.
   llvm::DIType getOrCreateRecordType(QualType Ty, SourceLocation L);

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

void CGDebugInfo::finalize() {
   // We keep our own list of retained types, because we need to look
   // up the final type in the type cache.
   for (std::vector<void *>::const_iterator RI = RetainedTypes.begin(),
          RE = RetainedTypes.end(); RI != RE; ++RI)
     DBuilder.retainType(llvm::DIType(cast<llvm::MDNode>(TypeCache[*RI])));
 
   DBuilder.finalize();
 }
+
+void CGDebugInfo::EmitExplicitCastType(QualType Ty) {
+  if (CGM.getCodeGenOpts().getDebugInfo() < CodeGenOptions::LimitedDebugInfo)
+    return;
+  llvm::DIType DieTy = getOrCreateType(Ty, getOrCreateMainFile());
+  // Don't ignore in case of explicit cast where it is referenced indirectly.
+  DBuilder.retainType(DieTy);
+}

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

public:
   Value *VisitInitListExpr(InitListExpr *E);
 
   Value *VisitImplicitValueInitExpr(const ImplicitValueInitExpr *E) {
     return EmitNullValue(E->getType());
   }
   Value *VisitExplicitCastExpr(ExplicitCastExpr *E) {
     if (E->getType()->isVariablyModifiedType())
       CGF.EmitVariablyModifiedType(E->getType());
+
+    if (CGDebugInfo *DI = CGF.getDebugInfo())
+      DI->EmitExplicitCastType(E->getType());
+
     return VisitCastExpr(E);
   }
   Value *VisitCastExpr(CastExpr *E);
 
   Value *VisitCallExpr(const CallExpr *E) {
     if (E->getCallReturnType()->isReferenceType())
       return EmitLoadOfLValue(E);