Page MenuHomePhabricator

Add observers to Input Graph
ClosedPublic

Authored by ruiu on May 12 2014, 8:32 PM.

Details

Summary

Make it possible to add observers to an Input Graph, so that files
returned from an Input Graph can be examined before they are
passed to Resolver.

To implement some PE/COFF features we need to know all the symbols
that *can* be solved, including ones in archive files that are not
yet to be read.

Currently, Resolver only maintains a set of symbols that are
already read. It has no knowledge on symbols in skipped files in
an archive file.

There are many ways to implement that. I chose to apply the
observer pattern here because it seems most non-intrusive. We don't
want to mess up Resolver with architecture specific features.
Even in PE/COFF, the feature that needs this mechanism is minor.
So I chose not to modify Resolver, but add a hook to Input Graph.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu updated this revision to Diff 9331.May 12 2014, 8:32 PM
ruiu retitled this revision from to Add observers to Input Graph.
ruiu updated this object.
ruiu edited the test plan for this revision. (Show Details)
ruiu added reviewers: Bigcheese, ruiu, shankarke.
ruiu added a subscriber: Unknown Object (MLST).
ruiu edited reviewers, added: atanasyan; removed: ruiu.May 12 2014, 8:35 PM
Bigcheese accepted this revision.May 12 2014, 10:12 PM
Bigcheese edited edge metadata.

lgtm.

This revision is now accepted and ready to land.May 12 2014, 10:12 PM
atanasyan edited edge metadata.May 12 2014, 11:25 PM

LGTM

include/lld/Core/InputGraph.h
74 ↗(On Diff #9331)

Maybe it is better to use the llvm::function_ref class to be consistent with main LLVM code.

http://llvm.org/docs/ProgrammersManual.html#passing-functions-and-other-callable-objects

Bigcheese added inline comments.May 13 2014, 12:57 AM
include/lld/Core/InputGraph.h
74 ↗(On Diff #9331)

This is news to me. I would much prefer we use std::function.

So after doing some research, it seems that advanced uses of std::function can fail on VC11 (VS 2012). We're not doing that here, and the buildbots have the final say in what is supported.

shankarke edited edge metadata.May 13 2014, 6:14 AM

Can you also add a test ? Possibly two tests ? One with InputGraphTest and one with YAML ?

include/lld/Core/InputGraph.h
74 ↗(On Diff #9331)

You might want to change the observer function to return an ErrorOr,

lib/ReaderWriter/FileArchive.cpp
100–108 ↗(On Diff #9331)

This may be put in one line using std::transform.

std::transform(_symbolMemberMap.begin(),_symbolMemberMap.end(), std::back_inserter(ret), [](const auto &pair){return pair.first;});

+richard, who added the current guidance in r208067.

ruiu updated this revision to Diff 9361.May 13 2014, 12:48 PM
ruiu edited edge metadata.
  • Added a test
ruiu added inline comments.May 13 2014, 12:57 PM
include/lld/Core/InputGraph.h
74 ↗(On Diff #9331)

I'd prefer std::function too.

74 ↗(On Diff #9331)

Return type of void is intentional. Observers should never fail, and should not do anything other than observing.

lib/ReaderWriter/FileArchive.cpp
100–108 ↗(On Diff #9331)

It looks more cryptic than the plain for loop. I think it's better to keep the for loop. It might make sense to write it in four lines like this.

std::set<StringRef> ret;
for (const auto &e : _symbolMemberMap)
  ret.insert(e.first)
return ret;
ruiu updated this revision to Diff 9368.May 13 2014, 5:15 PM
  • Replaced std::function with function_ref
atanasyan accepted this revision.May 13 2014, 10:08 PM
atanasyan edited edge metadata.

LGTM

ruiu closed this revision.May 19 2014, 7:23 AM
ruiu updated this revision to Diff 9555.

Closed by commit rL208753 (authored by @ruiu).