This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Don't mark forward declarations as canonical.
ClosedPublic

Authored by JDevlieghere on Aug 24 2017, 4:12 PM.

Details

Summary

This patch completes the work done by Frederic Riss (@friss) to
addresses dsymutil incorrectly considering forward declaration as
canonical during uniquing. This resulted in references to the forward
declaration even after the definition was encountered.

In addition to the test provided by Alexander Shaposhnikov (@alexshap)
in D29609, I added another test to cover several scenarios that were
mentioned in his conversation with Fred. We now also check that uniquing
still occurs after the definition was encountered.

For more context please refer to D29609

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Aug 24 2017, 4:12 PM
aprantl edited edge metadata.Aug 24 2017, 8:00 PM

mostly nitpicking :-)

test/tools/dsymutil/X86/odr-fwd-declaration.cpp
113

broken

114

referenced

tools/dsymutil/DwarfLinker.cpp
195

is there a more accurate description that "point to"? I.e., does this include a struct that has a fwddecl as a member?

1202

document the return value?

2210

extra newline? not sure if this was intentional

2261

maybe is complete?

2286

what does the return value mean?

@JDevlieghere, @friss, @aprantl - many thanks for working on this and getting the fix to the upstream!

JDevlieghere marked 5 inline comments as done.

Thanks for the review @aprantl! About the comment for the Incomplete member: can this strictly occur for members or has this a broader scope?

friss edited edge metadata.Aug 29 2017, 8:57 AM

A few nits, but this mostly LGTM

test/tools/dsymutil/X86/odr-fwd-declaration2.cpp
27

Add a CHECK-NOT: {{DW_TAG|NULL}} before this

86

Add the same CHECK-NOT here

99

You are reusing REF1. I suppose this those the right thing (overwrites it), but please use a different name.

135–136

As the bellow doesn't really test bPTr, bRef and bPtrToField, I would rephrase this as:
"Finally we confirm that uniquing isn't broken by checking that further references to 'struct A' point to its now complete definition.

tools/dsymutil/DwarfLinker.cpp
195

If it fits, "Does this DIE transitively refer to an incomplete decl?" would be better I think

Thanks @friss, I've updated the diff with your feedback.

friss accepted this revision.Aug 31 2017, 9:32 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 31 2017, 9:32 AM
This revision was automatically updated to reflect the committed changes.