This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Write TYPE_IDs for types used in functions imported by aliases
ClosedPublic

Authored by vitalybuka on Sep 17 2018, 6:00 PM.

Details

Summary

ThinLTO imports alias as a copy of a aliasee, so when we import such functions with type tests we will
need type ids used by function. However after D49565 we pick types only during processing of
FunctionSummary which is not happening for such aliesees.

Example:
Unit U1 with a type, a functions F with the type check, and an alias A to the function.
Unit U2 with only call to the alias A.

In particular, this happens when we use -mconstructor-aliases, which is default.
So if c++ unit only creates instance of the class, without calling any other methods it will lack of
necessary type ids, which will result in false CFI reports.

Diff Detail

Event Timeline

vitalybuka created this revision.Sep 17 2018, 6:00 PM
vitalybuka added a subscriber: pcc.
vitalybuka edited the summary of this revision. (Show Details)Sep 18 2018, 11:28 AM

Can you update description to point out that this is an issue because we will import the alias as a copy of aliasee?

Out of curiosity, what is the failure mode with this bug?

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
3881

We have already invoked AS->getAliasee() above (which would assert if there was no aliasee). So this check shouldn't be needed.

llvm/test/ThinLTO/X86/cfi-distributed.ll
24

Typo in RUN. But it looks like this RUN shouldn't be here as I don't see any COMBINED checks?

27

Comment needs updating.

34

This comment seems wrong - I think it should be "should only contain the typeid for _ZTS1A2" (which is what we're checking) - can you fix that? I think the comment/check will also need an update for your change - it should also contain the typeid for _ZTS1B2, right?

vitalybuka edited the summary of this revision. (Show Details)Sep 19 2018, 11:08 AM
vitalybuka marked 4 inline comments as done.
vitalybuka edited the summary of this revision. (Show Details)

Addressed comments from @tejohnson

llvm/test/ThinLTO/X86/cfi-distributed.ll
34

_ZTS1A3, done

Can you update description to point out that this is an issue because we will import the alias as a copy of aliasee?

Out of curiosity, what is the failure mode with this bug?

False CFI errrors. I've just added that into description.

This revision is now accepted and ready to land.Sep 19 2018, 11:36 AM
This revision was automatically updated to reflect the committed changes.
vitalybuka marked an inline comment as done.