On Fri, Dec 5, 2014 at 2:24 PM, David Malcolm <dmalc...@redhat.com> wrote: > What's the use-case here? Sorry if I'm not getting at what this is for.
The use case is that a program wants to use library functions, something common, not everything is self-contained and linked in automatically (like libc). Currently you would have to rely on the fact that a DSO can be created with dangling references which are expected to be somehow fulfilled at runtime. There are multiple problems with this: First, even if the application using the JIT itself is linked against a library which the JIT-generated code wants to use it is a problem if the definitions are accidentally found. If the library with the desired function in question uses symbol versioning the JIT-created DSO would have just an ordinary UNDEF entry for the symbol with no symbol version available. This then means that at runtime the /oldest/ version is picked. That not what you want in this case. Second, if you implement some form of extension language where the language allows to reference functions in other DSOs, you'd have to either use dlopen(RTLD_GLOBAL) in the main app (evil, ever use RTLD_GLOBAL) or you'd have to implicitly have the generated code use dlopen() and the dlsym(). That's cumbersome at best and also slow. On the other hand, with an option as proposed the code generator could simply record the dependency and have the DSO automatically used at link-time and runtime, creating the correct references etc. > Is the "self-containedness of the DSO" in your patch aimed at ensuring > that libpng.so.N gets unloaded when fake.so is unloaded? The unloading part is a nice additional benefit. It's mostly about the possibility to make it easily and quickly possible to call any function from any available DSO without having to know which DSOs are needed at the time the application using the JIT is linked. > One issue here is the lifetime of str options; currently str options > simply record the const char *, without taking a copy of the underlying > buffer. We might need to change this to make it take a strdup of the > option, to avoid nasty surprises if someone calls set_str_option with a > std::string and has it auto-coerced to a const char * from under them. I'm fine with that, I just followed what you did so far. If you want it done this way I'll add this to the patch. > New options should be documented in: > gcc/jit/docs/topics/contexts.rst in the "Options" section. > and these ones should probably be mentioned in the subsection on > GCC_JIT_FUNCTION_IMPORTED in functions.rst. I was more concerned with the code first... ;-) > Do you have a sense of what impact setting the option would have on the > time taken by gcc_jit_context_compile? It's really not much. The linker just tries different sizes for a hash table and picks the size with the least number of conflicts and therefore hopefully best performance at runtime. With today's machines this isn't really noticeable. Jakub (if you read this), when did we implement this? It still might not be a good idea to enable it by default and, as written, there might be other optimizations which are implemented. > This doesn't support nested contexts; presumably this should walk up > through any parent contexts, adding any linkfiles requested by them? Nested contexts? Do you deal with with gcc_jit_contact structures recursively? I must miss that. This is just a way to add more strings (free-form parameters) to the linker command line. I'm using ctxt.set_str_option(GCC_JIT_STR_OPTION_LINKFILE, "-lsomelibrary"); to have fake.so linked against libsomelibrary.so. > Here's another place where nested contexts may need to be supported: a > playback context's m_recording_ctxt may have ancestors, and they might > have linkfiles specified. This isn't the playback context structure, it the toplevel (gccjit::context) one. As far I can see there is no hierarchy and this makes sense. > I notice that this string option works differently from the others, in > that it appends to a list, rather than overwriting a value; that would > need spelling out in the documentation. Yes, sure, documentation is nothing I've concerned myself at that point. > I wondered if this should take a std::string instead of a const char *, > but a const char * is probably more flexible, given that you can go > trivially from a std::string to a const char *, but going the other way > may cost some cycles. If we want to make copies anyway I think it doesn't matter. I think using const char* is easier to use for the reasons you spelled out. > This descriptive comment needs fleshing out. For example, are these > filenames, or SONAMEs? How does this relate to what a user would pass > to the linker command line if they were writing a Makefile rather than > code that's calling into a JIT API? The strings are supposed to be exactly what you would add to the linker command line. No magic. In fact, the same mechanism can be used to pass arbitrary other options to the linker. These are just strings. An open question therefore is: should that be prohibited? Doubtful in my mind since it's not really possible to distinguish between linker options and filenames. "-d" is a valid file name. And hardcoding all known linker options and not allowing them is so against good practices. Aside, it might be necessary to define runpath using -Wl,-R,some/path. > It's usually best to give a concrete example. In the comments? In the documentation for sure. > The name of the option could be made more descriptive; if the intent > here is that this is an additional thing to link to, could this be: > GCC_JIT_STR_OPTION_EXTRA_LIBRARY > or somesuch? But it won't always be libraries. Maybe it's a .o file. > Or could this be a new API entrypoint altogether e.g. > > gcc_jit_context_requires_library (gcc_jit_context *ctxt, > const char *soname); That's doable. I was playing with the idea but it's less flexible. It would also means that another interface to add .o files should be added. The benefit is that it could be checked at the time of the call whether the respective file is what the caller wants to use it for and whether it exists in the first place. On the plus side, this would allow adding an extra parameter to the interface to specify at the same time the runpath. > One other issue here is that I've deliberately been a bit coy in the > user-facing documentation about the fact that the linker runs at all. > The assembler and linker show up in the profile, and at some point it > may be worth embedding them in-process somehow, so I wanted to try to > keep some of this as an implementation detail that's subject to change. > The user-facing docs do mention that a .so file is created. Understandable and this is what I wrote the (yet unfinished) code in elfutils for. Both the linker and assembler code can trivially be changed into libraries. So, a general "pass a string" option might indeed give rise to some unintended uses. So perhaps adding the two new interfaces is indeed what we should do. If you agree, I can hack that up.
