Page MenuHomePhabricator

LowerTypeTests: Add limited support for aliases
ClosedPublic

Authored by vlad.tsyrklevich on Jan 4 2018, 2:08 PM.

Details

Summary

LowerTypeTests moves some function definitions from individual object
files to the merged module, leaving a stub to be called in the merged
module's jump table. If an alias was pointing to such a function
definition LowerTypeTests would fail because the alias would be left
without a definition to point to.

This change 1) emits information about aliases to the ThinLTO summary,

  1. replaces aliases pointing to function definitions that are moved to

the merged module with function declarations, and 3) re-emits those
aliases in the merged module pointing to the correct function
definitions.

The patch does not correctly fix all possible mis-uses of aliases in
LowerTypeTests. For example, it does not handle aliases with a different
type from the pointed to function.

The addition of alias data increases the size of Chrome build artifacts
by less than 1%.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc added a comment.Jan 4 2018, 4:19 PM

Please add some negative tests, e.g. alias of function without jump table entry,

lib/Transforms/IPO/LowerTypeTests.cpp
963 ↗(On Diff #128651)

if (auto *A = dyn_cast<GlobalAlias>(U.getUser())) {

966 ↗(On Diff #128651)

I think you want to RAUW the alias with the newly created function declaration, otherwise this will fail if the alias has any uses. See line 775 in this file for an example.

1705 ↗(On Diff #128651)

Is it possible for ExportedFunctions.count(AliasName) to be false in a case where we need to create an alias in the merged module? It seems possible, e.g. if the alias is referenced from a native object file or a translation unit compiled without cfi-icall.

1711 ↗(On Diff #128651)

Similarly to my comment above, I think this will need to be a RAUW at the point where the alias is created in case the function declaration has any uses in the merged module.

vlad.tsyrklevich marked 3 inline comments as done.

Add negative tests and fix the bugs Peter pointed out.

lib/Transforms/IPO/LowerTypeTests.cpp
1711 ↗(On Diff #128651)

I don't think it's possible for there to be any uses here since this function declaration is created in the code directly above and can not yet have any uses and none should be generated below (maybe I'm missing something about how such a use would be generated?)

pcc added inline comments.Jan 5 2018, 3:59 PM
lib/Transforms/IPO/LowerTypeTests.cpp
1711 ↗(On Diff #128651)

The merged module may contain an ordinary reference to the function in user code (user code will appear in the merged module if it was compiled with full LTO or if it does not export any symbols). In that case we would fail the if condition on line 1654 above and reuse the existing function.

vlad.tsyrklevich marked an inline comment as done.

RAUW the alias we replace the function declaration with (as Peter mentioned in his comment, it's possible there are uses of it.)

pcc added a comment.Jan 9 2018, 12:32 PM

The code looks good, but please add negative tests as well as tests for the functionality you are adding to ThinLTOBitcodeWriter.

pcc added a comment.Jan 9 2018, 12:39 PM

Sorry disregard the negative tests comment, i see that you added some to the existing tests.

Add a ThinLTO bitcode writer test and fix a trivial UaF I found with an ASan test build.

pcc accepted this revision.Jan 9 2018, 3:09 PM

LGTM

This revision is now accepted and ready to land.Jan 9 2018, 3:09 PM
This revision was automatically updated to reflect the committed changes.