This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.
ClosedPublic

Authored by akhuang on Feb 26 2020, 3:02 PM.

Details

Summary

This change modifies the logic for adding CxxReturnUdt so that the option is
added to all member functions that return a record type, whether or not the
record type is marked as nontrivial. This matches MSVC's behavior.

This is to fix an issue where function types sometimes appeared twice in the
PDB: once with "returns cxx udt" and once without
(https://bugs.llvm.org/show_bug.cgi?id=44785).

There can also be cases where non-member functions that reference forward
declared record types result in duplicated types, but this is apparently not very
common.

Diff Detail

Event Timeline

akhuang created this revision.Feb 26 2020, 3:02 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 26 2020, 3:02 PM
rnk added inline comments.Feb 26 2020, 3:28 PM
clang/lib/CodeGen/CGDebugInfo.cpp
992 ↗(On Diff #246840)

One alternative would be to add llvm::DINode::FlagNonTrivial here if the type is complete and not trivial.

There is the case where the the compiler has to emit a method type when the class has been forward declared, and that could still lead to duplicate records. For example:

class Returned;
class Foo {
  Foo();
  Returned bar();
};
Foo::Foo() {}

In this example, clang will have to describe the method type of Foo::bar, but it will not have access to Returned. I think in practice this won't come up, because Foo::bar will be defined in the same file as the constructor.

llvm/include/llvm/IR/DebugInfoFlags.def
61 ↗(On Diff #246840)

@dblaikie @aprantl, does this seem like a reasonable flag to add, or should we mark record forward decls as trivial/nontrivial instead?

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
407

Should we assert here that the composite type is not a forward decl, assuming we stick with the CxxReturn flag?

428

We also use isNonTrivial here, but I think if we are emitting a method type, it means the class must not be a forward declaration?

akhuang marked an inline comment as done.Feb 26 2020, 5:19 PM
akhuang added inline comments.
clang/lib/CodeGen/CGDebugInfo.cpp
992 ↗(On Diff #246840)

I suppose this would be a lot simpler.

akhuang updated this revision to Diff 247104.Feb 27 2020, 1:56 PM

Just change CodeViewDebug to add CxxReturnUdt to all methods that return a record type,
which appears to be consistent with what msvc does and avoids emitting two versions of
the method in the pdb file.

rnk accepted this revision.Feb 28 2020, 5:35 PM

lgtm!

So, presumably we still do the wrong thing in the non-method type cases. I came up with this, where we differ with MSVC:

struct NonTrivial {
  NonTrivial();
  NonTrivial(const NonTrivial &o);
  ~NonTrivial();
  int x;
};
struct Foo {
  NonTrivial (*fp)(NonTrivial &o);
};
Foo gv;

MSVC marks the LF_PROCEDURE with the flag, but clang doesn't.

It looks like it NonTrivial is incomplete (we have a fwd decl DICompositeType), MSVC just marks it with CxxReturnUdt:

struct NonTrivial;
struct Foo {
  NonTrivial (*fp)(NonTrivial &o);
};
Foo gv;

$ cl -c t.cpp -Z7  && llvm-pdbutil dump -types t.obj | grep -A3 PROCEDU
Microsoft (R) C/C++ Optimizing Compiler Version 19.24.28315 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

t.cpp
0x1003 | LF_PROCEDURE [size = 16]
         return type = 0x1000, # args = 1, param list = 0x1002
         calling conv = cdecl, options = returns cxx udt

Now that I've gone this far... maybe we should just say isNonTrivial() || isFwdDecl() -> mark it CxxReturnUdt. Something like that.

This revision is now accepted and ready to land.Feb 28 2020, 5:35 PM
rnk added a comment.Mar 2 2020, 3:54 PM

BTW, I am curious to know if this helps V8 PDB size. IIRC you said there were some PDBs in Chrome that have this problem a lot.

In D75215#1899224, @rnk wrote:

Now that I've gone this far... maybe we should just say isNonTrivial() || isFwdDecl() -> mark it CxxReturnUdt. Something like that.

I just tried this but apparently it ends up marking things as CxxReturnUdt when the struct is trivial but not required to be complete--

So it seems like adding the NonTrivial flag to undefined forward declared functions would work.

akhuang updated this revision to Diff 247774.Mar 2 2020, 6:13 PM

Add FlagNonTrivial to fwd declared records in CGDebugInfo

akhuang updated this revision to Diff 247937.Mar 3 2020, 9:33 AM

update tests

In D75215#1901961, @rnk wrote:

BTW, I am curious to know if this helps V8 PDB size. IIRC you said there were some PDBs in Chrome that have this problem a lot.

I think I just noticed that the PDB size increased after enabling constructor type homing.

size of v8.dll.pdb
-debug-info-kind=limited: 175M
-debug-info-kind=constructor: 185M
-debug-info-kind=constructor + this patch: 169M

rnk added a comment.Mar 3 2020, 11:51 AM

I think we should commit the change to clang separately from the first one you made to LLVM to handle method types. Setting flags on forward declarations is something I'd like to get additional input on before landing.

clang/lib/CodeGen/CGDebugInfo.cpp
989 ↗(On Diff #247937)

I think this code block needs a comment about the behavior we are trying to implement. It could be made CodeView-specific, but I don't see a strong reason not to set these flags when emitting DWARF.

990 ↗(On Diff #247937)

So, we're marking forward declarations here as nontrivial to match MSVC, but we don't really know if they will end up being trivial or not. I guess that's fine.

clang/test/CodeGenCXX/debug-info-flag-nontrivial.cpp
2 ↗(On Diff #247937)

Please add a C-only test like this:

struct Incomplete;
struct Incomplete (*func_ptr)() = 0;

We shouldn't mark this struct as nontrivial, for example. The code you wrote is correct, but I wanted to test this corner case.

akhuang updated this revision to Diff 248001.Mar 3 2020, 12:59 PM

Splitting the forward declaration flag into a different review.

akhuang marked an inline comment as done.Mar 3 2020, 1:00 PM
akhuang added inline comments.
clang/lib/CodeGen/CGDebugInfo.cpp
989 ↗(On Diff #247937)

Yep, will add a comment.

akhuang updated this revision to Diff 248003.Mar 3 2020, 1:03 PM

add back line in test that accidentally got deleted

rnk accepted this revision.Mar 3 2020, 1:33 PM

lgtm, but please remove the clang test update from this patch, since it shouldn't pass yet.

clang/test/CodeGenCXX/debug-info-flag-nontrivial.cpp
1 ↗(On Diff #248003)

This test shouldn't be part of the LLVM-only change.

akhuang marked an inline comment as done.Mar 3 2020, 1:51 PM
akhuang updated this revision to Diff 248025.Mar 3 2020, 1:58 PM

remove tests

This revision was automatically updated to reflect the committed changes.
akhuang edited the summary of this revision. (Show Details)Mar 3 2020, 3:07 PM
dblaikie added inline comments.Mar 4 2020, 10:56 AM
llvm/include/llvm/IR/DebugInfoFlags.def
61 ↗(On Diff #246840)

Currently we only have a trivial/nontrivial flag that goes one way, right? (ie: true/false, not three state true/false/unknown)

That would present a problem for forward declarations - because for a true forward decl you can't know if it's trivial/non-trivial for passing, right? so that'd present a subtle difference between trivial/non-trivial on a decl (where it might be trivial/unknown) and on a def (where it's trivial/non-trivial), yes?

aprantl added inline comments.Mar 4 2020, 11:27 AM
llvm/include/llvm/IR/DebugInfoFlags.def
61 ↗(On Diff #246840)

Should this perhaps be a DI_SPFLAG instead?

rnk added inline comments.Mar 4 2020, 11:31 AM
llvm/include/llvm/IR/DebugInfoFlags.def
61 ↗(On Diff #246840)

It's true that there is an ambiguity between known trivial, known non-trivial, and forward declared, unknown triviality. Amy's solution to this problem was to mark forward declarations as nontrivial, which matches the logic MSVC uses to set CxxReturnUdt, but might not be the right thing for other consumers.

akhuang marked an inline comment as done.Mar 5 2020, 9:41 AM
akhuang added inline comments.
llvm/include/llvm/IR/DebugInfoFlags.def
61 ↗(On Diff #246840)

Alternatively, I guess we can FlagNonTrivial to FlagTrivial, and then we can mark all the unknown things with cxxreturnudt.

akhuang marked an inline comment as done and an inline comment as not done.Mar 5 2020, 11:06 AM
akhuang added inline comments.
llvm/include/llvm/IR/DebugInfoFlags.def
61 ↗(On Diff #246840)

maybe this doesn't work because then it would also mark C structs with cxxreturnudt.