Make DynamicLibrary::getPermanentLibrary have a defined ordering.
Needs ReviewPublic

Authored by marsupial on Feb 17 2017, 11:25 AM.



This is mostly affecting usage of the JIT...The best example is for Windows, but this makes symbol lookup the same on all platforms.

When using DynamicLibrary::getPermanentLibrary(0,0) to get a list of the loaded libs, this is currently stored in a set which makes iteration unordered / undefined. Additionally there is a problem as newer Windows are using ucrt.dll for a lot of stdlib functions which cause a disagreement between the JIT and native code as to what the address and implementation of that function is:

JIT: putenv_s("TEST", "VALUE") calls msvcrt.dll, putenv_s
JIT: getenv("TEST") -> "VALUE" calls msvcrt.dll, getenv
Native: getenv("TEST") -> NULL // calls ucrt.dll, getenv

The patch simply changes the way modules are stored.
Searches for symbols in reverse order so newer modules will override older ones.
And always tries to give priority to the process' symbols (what dlsym does I believe)

Diff Detail

marsupial created this revision.Feb 17 2017, 11:25 AM
marsupial edited subscribers, added: llvm-commits; removed: cfe-commits.Feb 17 2017, 11:42 AM
marsupial added a subscriber: cfe-commits.
marsupial removed a subscriber: cfe-commits.
marsupial edited the summary of this revision. (Show Details)Feb 19 2017, 5:52 PM
vsk added a subscriber: vsk.Feb 20 2017, 11:42 AM
vsk added a comment.Feb 20 2017, 3:03 PM

Some comments inline. Could you re-upload your diff with context ('git diff -U9999')?


Please use ManagedStatic.


Could you explain with some more detail why EnumProcessModulesEx needs to be repeated until its results don't change? Is there any guarantee that its results won't change again after this loop terminates?

Apologies if my Windows ignorance is showing here...


At this point, I'd really expect OpenedHandles to be initialized.


Try to avoid unrelated whitespace changes.

marsupial added inline comments.Feb 20 2017, 4:29 PM

Are you sure about that?
This code already has a lock guarding it.
What does ManagedStatic add?


There is a small chance of thread A calling EnumProcessModulesEx to get the size of the list and in between that and EnumProcessModulesEx to get the actual list that thread B does something to explicitly or implicitly load another dll.


Current has yet to be swapped into OpenedHandles there (line 90).


Ok, but there was a spurious indent there.

marsupial updated this revision to Diff 89161.Feb 20 2017, 10:16 PM

Wound up splitting most of it into a separate class. Predominately so ManagedStatic can reinitialize properly if necessary, but seems to provide a nicer separation and less code duplication when D29955 adds addPermanentLibrary

marsupial marked 8 inline comments as done.Feb 20 2017, 10:17 PM
marsupial updated this revision to Diff 89259.Feb 21 2017, 12:01 PM

The problem of undefined ordering was affecting Unix as well so moved to using an llvm::SetVector there.
Updated diff to latest changes.

marsupial updated this revision to Diff 89263.Feb 21 2017, 12:43 PM
marsupial retitled this revision from Make DynamicLibrary::getPermanentLibrary on Windows have a defined ordering. to Make DynamicLibrary::getPermanentLibrary have a defined ordering..
marsupial edited the summary of this revision. (Show Details)

Store and search for symbols the same way on Unix (in reverse and giving priority to the owning process' symbols).

marsupial edited the summary of this revision. (Show Details)Feb 21 2017, 8:18 PM
marsupial added reviewers: vsk, v.g.vassilev.
vsk added inline comments.Feb 22 2017, 2:28 PM

Maybe vector is the wrong container to use here, because it forces us to do O(n) work in one case. How about using a SmallPtrSet to do uniqueness testing, and a std::forward_list to preserve order?


Instead of having a 'KeepFront' switch, do you think it might be more convenient to store the RTLD_DEFAULT/IsProcess handle separately from the rest of the handles?


The convention is to write e.g '/*IsProcess=*/filename==nullptr', so it's clear which parameter you're passing in.


I'm hoping there's a way to reduce code duplication between the windows/unix logic. Have you tried factoring out some of the generic logic in HandleSet s.t it can call into platform-specific stubs?


If there isn't a guarantee that, e.g a third thread C couldn't load another dylib after this loop terminates, I don't think we should have the loop.

marsupial added inline comments.Feb 22 2017, 5:46 PM

I'm of the opinion that search will probably be fast enough and in the cases where it would't be using twice the memory is a worse penalty.


Seems reasonable. Hopefully I haven't already tried that and there is a reason not...


Yes there is a lot that can be reduced...just didn't want the patch to keep growing.
If this is the direction, I would probably add Unix/ and both the files would be the implementation of HandleSet.

Leaving DynamicLibrary.cpp the common implementation of the public class.


Generally I agree, but the cost of one compare that will most likely succeed to deal with an obscure event that would be hard to debug seems prudent?

marsupial added inline comments.Feb 22 2017, 6:04 PM

Ohh thats complicates the iterator interface!

marsupial updated this revision to Diff 89569.Feb 23 2017, 2:46 PM

Refactor platform specific code.
Remove Windows caching of libraries.
Add tests.

marsupial added inline comments.Feb 23 2017, 2:54 PM

Turns out the MSDN docs are pretty clear about not using the returned list if there has been a change. That is it may be possible for this to occur when [libA, libB, libC] are loaded:

Thread0: EnumProcessModulesEx: returns 3 for number of elements required
Thread0: EnumProcessModulesEx: called with HMODULE[3]
Thread1: FreeLibrary("libB")
Thread0: EnumProcessModulesEx: returns with [libA, libB, libC]

It looks like this patch is doing a lot more than what is described in the summary. I think it would make the review easier if you split the patch. I'd imagine that fixing the order of iteration is fairly minimal change and I'd recommend to start from there by opening another review request with just that.

I was asked to make the changes that grew it in size.
There doesn't seem to be an -easy- way to fix the issues at hand for a couple of reasons.
Just fixing the iteration order by using rbegin and rend is not enough, as the storage in a set is also a culprit.
Just fixing the Windows portion, means differing (and still broken) behavior on Unix.
The remaining broken behaviour on Unix means that the test cannot be added.

Though I do agree that this has stalled and do not know how to rectify that...

vsk resigned from this revision.Mon, Mar 6, 6:42 PM

I was asked to make the changes that grew it in size.

I did not mean to suggest you needed to tackle everything in one shot. As it stands, I don't have the bandwidth to review a patch of this size, and I won't hold up the review any longer.

marsupial updated this revision to Diff 92398.Mon, Mar 20, 4:23 PM