Page MenuHomePhabricator

dsymutil: Resolve forward decls for types defined in clang modules.
ClosedPublic

Authored by aprantl on Sep 21 2015, 5:18 PM.

Details

Reviewers
friss
Summary

This patch extends llvm-dsymutil's ODR type uniquing machinery to also resolve forward decls for types defined in clang modules.

Diff Detail

Event Timeline

aprantl updated this revision to Diff 35325.Sep 21 2015, 5:18 PM
aprantl retitled this revision from to dsymutil: Resolve forward decls for types defined in clang modules..
aprantl updated this object.
aprantl added a reviewer: friss.
aprantl set the repository for this revision to rL LLVM.
aprantl added a subscriber: llvm-commits.
aprantl updated this revision to Diff 35408.Sep 22 2015, 1:30 PM
aprantl removed rL LLVM as the repository for this revision.

Rebased patch.

friss edited edge metadata.Sep 22 2015, 4:43 PM

Basically looks good. Some comments/questions:

tools/dsymutil/DwarfLinker.cpp
1665–1677

I like the simplicity of adding modules as some kind of namespace. The fact that we hash the Tag here prevents us from uniquing what's in a module and standard namespaces. However the FIXME above only talks about backward compatibility. Maybe we should remove that FIXME and instead explain that it avoid conflicts between modules and other Decl scopes (and add a small note about preventing uniquing a struct with a class of the same name, but that this corner case doesn't warrant making that conditional).

1755–1756

We should really rename that and change the comment (it should have done before, but your patch makes it even more meaningless). Maybe gatherInitialDIEInfo ? Or gatherDIEScopes ? Pick whichever you want (or an even more awesome name that I couldn't come up with).

1805–1810

Wouldn't that logic always prune the TAG_module? or am I reading this wrong?

2223–2224

I would have expected Prune to override Keep. What is the '&& !AlreadyKept' handling?

aprantl added inline comments.Sep 22 2015, 5:02 PM
tools/dsymutil/DwarfLinker.cpp
1805–1810

It will prune all DW_TAG_modules that have no children or whose children were pruned.
But what about imported empty modules? (See below).

2223–2224

It's handling an otherwise pruned (but imported) DW_TAG_module.

aprantl updated this revision to Diff 35449.Sep 22 2015, 5:24 PM
aprantl edited edge metadata.

Address review comments.

tools/dsymutil/DwarfLinker.cpp
1665–1677

The fact that we hash the Tag here prevents us from uniquing what's in a module and standard namespaces.

Even if we wouldn't hash the Tag, we would still hash the module's name (NameRef) which is crucial in order to support (Obj)C because it is legal to have conflicting definitions for the same type in two differently named modules.

In order to support uniquing in C++ between module types and other types, we should change clang to not emit C++ types inside a DW_TAG_module instead.

1755–1756

I tried analyzeContextInfo(). We can still rename it if we can come up with something better.

friss added inline comments.Sep 22 2015, 6:14 PM
tools/dsymutil/DwarfLinker.cpp
1665–1677

The fact that we hash the Tag here prevents us from uniquing what's in a module and standard namespaces.

Even if we wouldn't hash the Tag, we would still hash the module's name (NameRef) which is crucial in order to support (Obj)C because it is legal to have conflicting definitions for the same type in two differently named modules.

Yes, I know that. I'm thinking of something different. Let's take a C++ program that uses a module called Foo. If the same program has a namespace called Foo (unrelated to the module), then we could have ODR uniquing collisions between unrelated things if we didn't hash the Tag.

I'm just saying that we should promote the FIXME comment from a bug I wanted to remove to a feature that's needed.

1755–1767

I tried analyzeContextInfo(). We can still rename it if we can come up with something better.

Sounds good.

1805–1810

It will prune all DW_TAG_modules that have no children or whose children were pruned.

Sorry I misread the code. Can you add a comment to the function explaining the meaning of the return value please?

friss added inline comments.Sep 22 2015, 6:18 PM
test/tools/dsymutil/X86/modules.m
59–73

Could you make all the CHECKs in this test a bit more rigorous? (some more CHECK-NOT DW_TAG|NULL)

aprantl updated this revision to Diff 35513.Sep 23 2015, 8:50 AM
  • made the tests stricter
  • clarified comments
tools/dsymutil/DwarfLinker.cpp
1665–1677

Got it now. I thought you were getting at that we aren't be able to unique types if one file imports a module and a second file is compiled without -fmodules and just #includes the module header.

aprantl updated this revision to Diff 35515.Sep 23 2015, 8:54 AM
  • add a comment about the return value of analyzeContextInfo().
friss accepted this revision.Sep 23 2015, 9:17 AM
friss edited edge metadata.

You'll need to to rebase to the latest sources, but this LGTM.

Thanks!

This revision is now accepted and ready to land.Sep 23 2015, 9:17 AM
aprantl closed this revision.Sep 23 2015, 1:47 PM

Thanks!
Committed as r248398