This is an archive of the discontinued LLVM Phabricator instance.

[ORC][COFF] Handle COFF import files of static archive.
ClosedPublic

Authored by sunho on Jul 16 2022, 9:20 PM.

Details

Summary

Handles COFF import files of static archive. Changes static library genrator to build up object file map keyed by symbol name that excludes the symbols from dllimported symbols so that static generator will not be responsible for them. It exposes the list of dynamic libraries that need to be imported. Client should properly load the libraries in this list beforehand. Object file map is also an improvment from the past in terms of performance. Archive.findSym does a slow O(n) linear serach of symbol list to find the symbol. (we call findSym O(n) times, thus full time complexity is O(n^2); we were the only user of findSym function in fact)

There is a room for improvements in how to load the libraries in the list. We currently just hand the responsibility over to the clinet. A better way would be let ORC read this list and hand them over to JITLink side that would also help validation (e.g. not trying to generate stub for non dllimported targets) Nevertheless, we will have to exclude the symbols from COFF import object file list and need a way to access this list, which this patch offers.

Diff Detail

Event Timeline

sunho created this revision.Jul 16 2022, 9:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 9:20 PM
sunho requested review of this revision.Jul 16 2022, 9:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 9:20 PM
sunho edited the summary of this revision. (Show Details)Jul 16 2022, 9:27 PM
sunho updated this revision to Diff 445290.Jul 16 2022, 9:37 PM
sunho updated this revision to Diff 445291.Jul 16 2022, 9:41 PM
sunho updated this revision to Diff 445315.Jul 17 2022, 4:45 AM
sunho updated this revision to Diff 445739.Jul 19 2022, 2:05 AM
sunho updated this revision to Diff 445740.
sunho updated this revision to Diff 445799.Jul 19 2022, 6:43 AM

Use symbolstringptr.

Does obj2yaml work for archives? If so it'd be cool to have a testcase for this. If obj2yaml doesn't support this I'm not sure we could count on having available tools that would produce an archive with COFF imports in it, could we?

llvm/include/llvm/ExecutionEngine/Orc/ExecutionUtils.h
290–295

Eventually I think we'll want this to be a separate structure returned by the Create, rather than a method on StaticLibraryDefinitionGenerator, but it's early days for this API -- I don't mind if it lands like this initially.

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
1679–1681

This is always going to load the library as a real dylib (via EPCDynamicLibrarySearchGenerator), right?

Could we have it treated as if it were a -L argument instead?

sunho added a comment.Jul 21 2022, 9:19 AM

obj2yaml tries to dump the archive files but support for lib file is broken. I think we can use other tools to genrate test cases though. (we could just use raw lib file if we can't get anything working)

sunho added inline comments.Jul 25 2022, 8:32 AM
llvm/tools/llvm-jitlink/llvm-jitlink.cpp
1679–1681

We want to search the dll file that is not inside specified -L serach directories as well. One way is extend existing -l flag to also look up system dynamic library if everything else fails in search directorires and print warning message. How does that sound to you?

lhames accepted this revision.Jul 25 2022, 3:27 PM
lhames added inline comments.
llvm/tools/llvm-jitlink/llvm-jitlink.cpp
1679–1681

How is MSVC / LINK.EXE handling this? Do they just have a fallback list of search paths to search on? I'd be fine with including those as if they were -L arguments by default (maybe with a switch to turn them off explicitly where they're not wanted)

This revision is now accepted and ready to land.Jul 25 2022, 3:27 PM
sunho added inline comments.Jul 26 2022, 7:49 AM
llvm/tools/llvm-jitlink/llvm-jitlink.cpp
1679–1681

It would just rely on windows kernel to do the dll search which is described in https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order. However, common use cases of llvm-jitlink wouldn't really want to look pathes other than MSVC installations and probably some windows system path. So, I think it's fine to go with adding some default path to -L way.

sunho updated this revision to Diff 447768.Jul 26 2022, 11:10 AM

New approach in llvm-jitlink

sunho updated this revision to Diff 447776.Jul 26 2022, 11:34 AM

Optimize code a bit. I don't think this part of code needs optimization but I couldn't help it.

sunho updated this revision to Diff 447777.Jul 26 2022, 11:34 AM
sunho updated this revision to Diff 447778.Jul 26 2022, 11:38 AM
sunho updated this revision to Diff 447784.Jul 26 2022, 11:50 AM
sunho updated this revision to Diff 447786.Jul 26 2022, 11:55 AM
lhames accepted this revision.Jul 26 2022, 6:07 PM

See individual comments, but otherwise LGTM!

llvm/lib/ExecutionEngine/Orc/ExecutionUtils.cpp
426

Following the usual error handling idioms you should add an ErrorAsOutParameter _(&Err); variable to the start of this function body -- this will clear the checked flag on exit from the constructor, forcing the caller to check it. (The original version did not need this, as Err was never modified in the body.)

In this case it's not necessary at the moment because the constructor is private and the only caller returns the Error (which will also clear the checked flag), but it would become important if we ever made the constructor public.

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
97–98

"dynlib" isn't used anywhere else -- I think this should either be "dylib", or just plain "search-system-lib-paths".

1272–1277

Could you add an TODO to move this logic into ObjectFile::getTriple().

1537–1546

We probably want to add API to the ORC runtime to retrieve the remote environment, rather than grabbing the local one. Could you add a TODO for this too?

1597

What's the rationale behind the switch to a priority queue? Unless there's a good reason for the switch this should stay as a std::vector, just to minimize the differences introduced in this patch.

1685–1686

This should generate an llvm::Error rather than assert -- we can't assume that object contents are well-formed.

1807–1812

Rather than hoisting the loop into a lambda, you could use LLVM's handy concat-iterator utility to iterate over both sequences in one go:

for (StringRef SearchPath : concat<StringRef>(JDSearchPaths[&JD], SystemSearchPaths)) {
    ..
}
sunho updated this revision to Diff 448513.Jul 28 2022, 10:27 PM
sunho marked 4 inline comments as done.Jul 28 2022, 10:29 PM
sunho added inline comments.
llvm/tools/llvm-jitlink/llvm-jitlink.cpp
1597

We still need another type of data structure to "push_front" We could reverse the order of sorting -- but that makes it more confusing in my opinion. I changed it to deque to keep it similar in terms of code and performance while enabling to push load job in the middle.

sunho updated this revision to Diff 448514.Jul 28 2022, 10:30 PM
sunho updated this revision to Diff 448516.Jul 28 2022, 10:38 PM
sunho marked 4 inline comments as done.
sunho marked an inline comment as done.Jul 28 2022, 10:41 PM
This revision was automatically updated to reflect the committed changes.