r281278 - [DebugInfo] Deduplicate debug info limiting logic

David Blaikie via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 12 17:20:29 PDT 2016


On Mon, Sep 12, 2016 at 5:09 PM Reid Kleckner via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Author: rnk
> Date: Mon Sep 12 19:01:23 2016
> New Revision: 281278
>
> URL: http://llvm.org/viewvc/llvm-project?rev=281278&view=rev
> Log:
> [DebugInfo] Deduplicate debug info limiting logic
>
> We should be doing the same checks when a type is completed as we do
> when a complete type is used during emission. Previously, we duplicated
> the logic, and it got out of sync. This could be observed with
> dllimported classes.
>

Thanks for having a go at this!

The logic in completeType (& maybe also completeClassData) still seems to
duplicate some of the logic in shouldOmitDefinition - I think the original
idea I had was a series of cascading entry points (completeType checking
the "oh, the type now has a definition", then falling into
completeRequiredType, etc)

In theory the "shouldOmitDefinition" could be split into the extra
conditions for each of the parts of that cascade, so the checks wouldn't be
redundantly executed when we were already being called about the completion
or required completion of a type, etc. But I'm not sure the performance
gain would be measurable, etc.

If not, we might as well just have one entry point that's something like
"revisit type because something interesting happened to it" (it became
complete, it became required to be complete, etc)... but that seems a bit
weird too.


>
> Also reduce a test case for this slightly.
>

The UseCompleteType is still there/unnecessary (as is the 'private' in
UnicodeString) - and I'd probably rename it to foo/bar and throw it in a
namespace like the rest for consistency - having semantic names in a test
when they're just remnants from whatever code it was reduced from seems
confusing to me. But I realize I'm a bit more pedantic about that than most.

- Dave


>
> Implementing review feedback from David Blaikie on r281057.
>
> Modified:
>     cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>     cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp
>     cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp
>
> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=281278&r1=281277&r2=281278&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Sep 12 19:01:23 2016
> @@ -1644,27 +1644,6 @@ void CGDebugInfo::completeType(const Rec
>      completeRequiredType(RD);
>  }
>
> -void CGDebugInfo::completeRequiredType(const RecordDecl *RD) {
> -  if (DebugKind <= codegenoptions::DebugLineTablesOnly)
> -    return;
> -
> -  // If this is a dynamic class and we're emitting limited debug info,
> wait
> -  // until the vtable is emitted to complete the class debug info.
> -  if (DebugKind <= codegenoptions::LimitedDebugInfo) {
> -    if (const auto *CXXDecl = dyn_cast<CXXRecordDecl>(RD))
> -      if (CXXDecl->isDynamicClass())
> -        return;
> -  }
> -
> -  if (DebugTypeExtRefs && RD->isFromASTFile())
> -    return;
> -
> -  QualType Ty = CGM.getContext().getRecordType(RD);
> -  llvm::DIType *T = getTypeOrNull(Ty);
> -  if (T && T->isForwardDecl())
> -    completeClassData(RD);
> -}
> -
>  void CGDebugInfo::completeClassData(const RecordDecl *RD) {
>    if (DebugKind <= codegenoptions::DebugLineTablesOnly)
>      return;
> @@ -1763,6 +1742,16 @@ static bool shouldOmitDefinition(codegen
>    return false;
>  }
>
> +void CGDebugInfo::completeRequiredType(const RecordDecl *RD) {
> +  if (shouldOmitDefinition(DebugKind, DebugTypeExtRefs, RD,
> CGM.getLangOpts()))
> +    return;
> +
> +  QualType Ty = CGM.getContext().getRecordType(RD);
> +  llvm::DIType *T = getTypeOrNull(Ty);
> +  if (T && T->isForwardDecl())
> +    completeClassData(RD);
> +}
> +
>  llvm::DIType *CGDebugInfo::CreateType(const RecordType *Ty) {
>    RecordDecl *RD = Ty->getDecl();
>    llvm::DIType *T = cast_or_null<llvm::DIType>(getTypeOrNull(QualType(Ty,
> 0)));
>
> Modified: cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp?rev=281278&r1=281277&r2=281278&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/debug-info-class-nolimit.cpp Mon Sep 12
> 19:01:23 2016
> @@ -6,10 +6,7 @@
>  // more general than that.
>
>  struct UnicodeString;
> -struct GetFwdDecl {
> -  static UnicodeString format;
> -};
> -GetFwdDecl force_fwd_decl;
> +UnicodeString *force_fwd_decl;
>  struct UnicodeString {
>  private:
>    virtual ~UnicodeString();
>
> Modified: cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp?rev=281278&r1=281277&r2=281278&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp
> (original)
> +++ cfe/trunk/test/CodeGenCXX/debug-info-dllimport-base-class.cpp Mon Sep
> 12 19:01:23 2016
> @@ -4,6 +4,10 @@
>  // be imported from a DLL.  Otherwise, the debugger wouldn't be able to
> show the
>  // members.
>
> +// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name:
> "ImportedAfterCompletion",
> +// CHECK-NOT:              DIFlagFwdDecl
> +// CHECK-SAME:             ){{$}}
> +
>  // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name:
> "OutOfLineCtor",
>  // CHECK-SAME:             DIFlagFwdDecl
>  // CHECK-SAME:             ){{$}}
> @@ -16,6 +20,13 @@
>  // CHECK-NOT:              DIFlagFwdDecl
>  // CHECK-SAME:             ){{$}}
>
> +
> +struct ImportedAfterCompletion;
> +ImportedAfterCompletion *force_fwd_decl;
> +struct __declspec(dllimport) ImportedAfterCompletion {
> +  virtual ~ImportedAfterCompletion();
> +};
> +
>  struct OutOfLineCtor {
>    OutOfLineCtor();
>    virtual void Foo();
> @@ -35,6 +46,7 @@ struct ImportedMethod {
>  };
>
>  int main() {
> +  ImportedAfterCompletion c;
>    OutOfLineCtor o;
>    DerivedFromImported d;
>    ImportedMethod m;
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160913/03b4d3fb/attachment.html>


More information about the cfe-commits mailing list