This is an archive of the discontinued LLVM Phabricator instance.

[lib/LTO] Remove now unneded hack for undefined symbols
AbandonedPublic

Authored by davide on Sep 14 2016, 5:04 PM.

Details

Summary

As already noted by Rafael, once https://reviews.llvm.org/D24594 will land, we won't need this anymore.

Diff Detail

Event Timeline

davide updated this revision to Diff 71464.Sep 14 2016, 5:04 PM
davide retitled this revision from to [lib/LTO] Remove now unneded hack for undefined symbols.
davide updated this object.
davide added reviewers: rafael, pcc, tejohnson.
davide added a subscriber: llvm-commits.
lib/LTO/LTO.cpp
341

I wonder if we should instead assert that we don't have an undefined symbol with a prevailing resolution.

I don't have a strong preference. Peter?

tejohnson added inline comments.Sep 14 2016, 5:34 PM
lib/LTO/LTO.cpp
341

That sounds like a great idea - that's what intrigued me in the first place and what I was trying to figure out in the debugger yesterday for PR30363.

I'll reformulate: what is the semantic of having a prevailing resolution for an undefined symbol?
If there is none we should error (or at least assert).

I'll reformulate: what is the semantic of having a prevailing resolution for an undefined symbol?
If there is none we should error (or at least assert).

I don't think an undefined can have a prevailing resolution, at least in lld and gold.

davide updated this revision to Diff 71469.Sep 14 2016, 5:49 PM

Add an assertion, as per Mehdi suggestion.

mehdi_amini added inline comments.Sep 14 2016, 5:55 PM
lib/LTO/LTO.cpp
387

We should have the same assert in this loop (maybe in a separate patch)

davide added inline comments.Sep 14 2016, 5:57 PM
lib/LTO/LTO.cpp
387

Oh, sure :) I'm still not very familiar with Thin so I didn't think about it. Thanks!

mehdi_amini added inline comments.Sep 14 2016, 6:02 PM
lib/LTO/LTO.cpp
356

Is this ok with Sym.getFlags() & object::BasicSymbolRef::SF_Undefined?
The continue; you're removing makes us no longer short-circuiting this code.

356

I guess we can't have Common and undefined at the same time...

pcc added inline comments.Sep 14 2016, 6:06 PM
lib/LTO/LTO.cpp
387

Or you could add it to addSymbolToGlobalRes.

tejohnson added inline comments.Sep 14 2016, 6:51 PM
lib/LTO/LTO.cpp
356

In any case, it will assert now instead of continuing, so we still wouldn't hit this for an undefined (you might be still looking at the older version of the patch).

387

Ah that's a good idea, better to consolidate it there and keep these loops simple.

So, are you OK with committing this assertion (after it's moved to addSymbolToGlobalRes())

mehdi_amini accepted this revision.Sep 16 2016, 10:56 AM
mehdi_amini added a reviewer: mehdi_amini.
This revision is now accepted and ready to land.Sep 16 2016, 10:56 AM

Looks like patch was not committed.

davide abandoned this revision.Oct 17 2016, 7:48 PM

The hack is still needed for lld, unfortunately. Abandoning.