Page MenuHomePhabricator

[Verifier] Verifier that a GlobalValue is only used in this Module
ClosedPublic

Authored by loladiro on Dec 6 2015, 5:31 PM.

Details

Summary

We already have the inverse verification that we only use globals
that are defined in this module. This essentially catches the
same mistake, but when verifying the module that contains the
definition. There is a moderately annoying corner case caused by
our usage of a parentless ReturnInst to hold prefix/prologue data.
In order to perform correct diagnostics, we need to construct the
inverse map for this on demand. Since that can be expensive, this
inverse mapping is cached in the Verifier class and only constructed
when needed.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 42024.Dec 6 2015, 5:31 PM
loladiro updated this revision to Diff 42025.
loladiro retitled this revision from to [Verifier] Verifier that a GlobalValue is only used in this Module.
loladiro updated this object.
loladiro added a reviewer: dexonsmith.
loladiro added a subscriber: llvm-commits.

Clang-format everything (sorry forgot before submitting)

loladiro updated this revision to Diff 43303.Dec 19 2015, 5:30 AM

Now that the ReturnInst hack is removed, this can be done much more cleanly.

Bump @dexonsmith, does this look ok now?

rafael added inline comments.
lib/IR/Verifier.cpp
451 ↗(On Diff #43303)

Why do you need a template? Looks like the callbacks always have the name type, no?

453 ↗(On Diff #43303)

Start function names with a lowercase letter.

456 ↗(On Diff #43303)

Factor TheUse.getUser() to a variable.

loladiro added inline comments.Dec 23 2015, 6:51 AM
lib/IR/Verifier.cpp
451 ↗(On Diff #43303)

Is there a good way to name a C++ lambda (I know about std::function, but AFAIK that has performance implications, so I tend to avoid it)?

loladiro updated this revision to Diff 43540.Dec 23 2015, 8:38 AM

Address review comments and rebase. Rebasing required changing a few uses() to
materialized_uses(). I hope that's the correct thing.

rafael added inline comments.Dec 23 2015, 9:47 AM
lib/IR/Verifier.cpp
455 ↗(On Diff #43540)

The user is the only thing this function uses.

Can you pass a "Value *User" instead and in the recursive call iterate over users() instead of uses()?
Can you add a comment describing what the function does (recursively wall every indirect user)?

460 ↗(On Diff #43540)

How about having a single callback that takes a Value and do the cast in the callback?

492 ↗(On Diff #43540)

This can also iterate over users, no?

loladiro added inline comments.Dec 23 2015, 10:05 AM
lib/IR/Verifier.cpp
455 ↗(On Diff #43540)

Yep.

460 ↗(On Diff #43540)

My concern was that I already have to check for the type here in order to see whether to call the callback or recurse, so I thought it was cleaner to have two callbacks, but I don't feel very strongly about that.

> How about having a single callback that takes a Value and do the cast

in the callback?

My concern was that I already have to check for the type here in order to

see whether to call the callback or recurse, so I thought it was cleaner to
have two callbacks, but I don't feel very strongly about that.

Why not always call the callback? We have a set to avoid a loop.

Cheers, Rafael

loladiro updated this revision to Diff 43547.Dec 23 2015, 10:24 AM

Switch uses() -> users() and add a comment to explain what the foreach helper is for.

loladiro updated this revision to Diff 43626.Dec 25 2015, 3:13 AM

Make the indirect user walker generic

Any more comments or is this good to go?

rafael accepted this revision.Jan 14 2016, 1:44 PM
rafael added a reviewer: rafael.

LGTM with a nit.

lib/IR/Verifier.cpp
499 ↗(On Diff #43626)

Nit: There are many calls to GV.getParent(). Don't you have the module being verified handy somewhere?

This revision is now accepted and ready to land.Jan 14 2016, 1:44 PM
This revision was automatically updated to reflect the committed changes.