[LLVMdev] 2nd attempt for a working patch for bug 2606

[Garrison Venn]

Hi Jeffrey,

I'm still in the process of learning the unittest technology. While I did get distracted with
test-suite and test, and therefore the requisite installs of llvm-gcc, and DejaGNU, I think
I'm back on track now. So that was a waste of time. By the way the reference I have to the 
unittests target was in an archive for this list. Is there no llvm level doc for this? Regardless
the gtest.h is fairly easy to understand. It just seems like one can get distracted by the 
lack of llvm doc. on unittests when a corresponding doc exists for the testing infrastructure. 
The developers guide did not seem to contain anything either. If there is such a doc, could 
you give me a link?


On Feb 26, 2010, at 21:17, Jeffrey Yasskin wrote:

> 
> You currently have nearly the same loop in getOrEmitGlobalVariable()
> and getPointerToFunction(). Can you unify those, perhaps by making
> them searches for GlobalValues with getNamedValue()?
> 
> GetLinkageResult() in lib/Linker/LinkModules.cpp has a lot of logic
> about choosing a function based on its linkage. You should read that
> to figure out how to filter GVs with various linkages in your search.

When I'm able to write a unit test, I'll re-submit a patch which will contain
a JIT function to handle GlobalValues (both GlobalVariables and Functions).
I did this for that CrossModuleJIT experiment that I posted in another thread.
Anyway I'll start from there, and then proceed to using LinkModules.cpp logic.

[snip]
> 
> Please add some tests for cross-module JITting. You'll find some of
> the functions defined in unittests/ExecutionEngine/JIT/JITTest.cpp
> helpful, but I think this belongs in a separate file. Just copy the
> useful functions, and at some point I'll build an actual library for
> the helper functions.

Working on this now.

[snip]
> 
> I suspect you can pull the decision of how to emit a stub back into
> the JITEmitter. If you add a
> "Function*findDefiningFunction(Function*)" method to the JIT, which
> searches the Modules for the strongest definition of a same-named
> function, you can keep the logic for how to deal with compiling that
> function in the JITEmitter.

Will do
> 
>> I also kept the hasAvailableExternallyLinkage() the same as
>> before, as I'm not sure if this
>> should also go through the same module procedure.
> 
> If something's available_externally in the current module and external
> in another module, I think we should compile the external definition.
> If it's available_externally in all modules, we need to look with
> dlsym(). Please add a test for that.

Will do
> 
>> The stub emission will
>> only take place for function
>> definitions that are marked with external linkage. Should other cases be
>> allowed? Should visibility also
>> be considered?
> 
> Yes, I think you should do the same thing LinkModules.cpp does when
> it's possible.

Yes I'll create one function to treat all GlobalValues and then reuse LinkModules
logic. I've yet to look at LinkModules.cpp

[snip]

> 
> Could you add a test in which M1::F1 uses M2::GVar1, which contains in
> its initializer the address of M1::F2?
> 
Will do

>> An attempt is first made to find the global outside of all modules, before
>> attempting to find the function
>> in the known modules. This is the reverse of logic used in #2.
> 
> That doesn't seem right.

Ok I'll check on my understanding. On thinking about it, I think I see how I'm 
mistaken.

> 
>> On searching,
>> unlike for functions, a linkage
>> check was not made. Also the global variable declaration and definition must
>> have the exact same type.
>> Is this the correct approach?
> 
> They may not have exactly the same type because of opaque types in
> each module. We probably shouldn't go to the trouble of unifying the
> types between the modules, so I'd be inclined to just ignore the
> types. Or maybe only pay attention when they're concrete types?

Ok, I'll modify accordingly if I understand how.

> 
>> 4)    The declaration of void* forceEmitFunctionStub(Function *F) was added
>> to the class JIT. This is
>> implemented in JITEmitter.cpp.
> 
> This isn't quite the right name, but I think you can eliminate it
> entirely (see above).

Yup
> 
>> Beyond any issues with the patch, there is a question, in my mind, as to
>> whether 2606 is really a bug.
> 
> I suspect it is.

Cool, then we are moving forward.

> 
>> Sure its resolution makes
>> using the JIT simpler for cross module behavior, but current manual
>> solutions may be more fined grained in their approach in
>> deciding how to treat such functions. If true a fix to 2606 would circumvent
>> such handling.
> 
> If I understand correctly, the current manual solution is to figure
> out what functions the JIT is going to want from another module, and
> compile them first. That would continue to work even after the JIT
> links modules for you.

Yes, in re-thinking this, I believe you are correct. I'm not sure about
all the use cases though, but you have much more knowledge on this
than I do. So I'll ignore and proceed.

> 
>> Should we instead add a new
>> semantic to EngineBuilder which would configure the JIT, or whatever else,
>> to handle such cross module linkage, if such behavior
>> is warranted? By linkage I mean mapping variables, and functions found in
>> "external" modules, into the execution engine. For example,
>> one could devise a new function pass style metaphor which would be setup in
>> the EngineBuilder instance. If it exists, the resulting
>> JIT instance would use this new pass style to determine what to do with
>> "foreign functions, and variables".
> 
> This strikes me as overkill until someone actually needs it.

Yes, most definitely
> 
> 
> Thanks,
> Jeffrey

Thanks again. By the way if I'm too slow on this, don't hesitate to jump in. I have to
learn this process anyway, and this bug is a good way to do it. So I'll proceed in my
own private space even if you, Oliver, or someone else shotguns ahead. 

I do have one question though. After having done the fix and the appropriate unit tests, 
does one still need to add to the tests suite for the testing bots, and add to test for make 
check (for this kind of bug fix)? Do the unittests somehow get invoked by make check? I 
guess I don't really understand the relation between the tests exercised by the unittests 
target, and the tests exercised by the check target. Maybe there isn't any.

Garrison