Today I found a nice bug in the JIT global variable emitter.
The problem may lead to an assert() failure when doing the following:
1) compile some function
2) emit a global variable
3) compile another function. an assert() may trigger in the JIT memory manager
This happens because the JIT global variable emitter is using the MachineCodeEmitter::allocate() function, which uses memory allocated by the JIT memory manager (which should be used for functions only). The assert() is triggered because the global variable is dumped to the header of the free blocks list (provided that ThisAllocated becomes 1).
Today I found a nice bug in the JIT global variable emitter.
The problem may lead to an assert() failure when doing the following:
1) compile some function
2) emit a global variable
3) compile another function. an assert() may trigger in the JIT memory
manager
This happens because the JIT global variable emitter is using the
MachineCodeEmitter::allocate() function, which uses memory allocated by the
JIT memory manager (which should be used for functions only).
No, this was a deliberate change, 54442. We have a situation where a user
wants to emit JIT code on one machine, then send it off to another machine to
execute. Putting statically allocated data in the same buffer as code is the
easiest approach to make this work, although there may be others.
Is this the known situation that the JIT memory manager doesn't handle buffer overflow
well? You could get into problems with that when it contained only code, although
it obviously gets hit more often now.
Today I found a nice bug in the JIT global variable emitter.
The problem may lead to an assert() failure when doing the following:
1) compile some function
2) emit a global variable
3) compile another function. an assert() may trigger in the JIT memory
manager
This happens because the JIT global variable emitter is using the
MachineCodeEmitter::allocate() function, which uses memory allocated
by the
JIT memory manager (which should be used for functions only).
No, this was a deliberate change, 54442. We have a situation where a
user
wants to emit JIT code on one machine, then send it off to another
machine to
execute. Putting statically allocated data in the same buffer as code
is the
easiest approach to make this work, although there may be others.
Ok, thanks for the explanation. So my first patch doesn't work. Also, to be clear, this bug has nothing to do with overflowing the JIT memory buffer.
I made another one that takes keeps the allocation of global variables in the JIT buffer, but it creates a new mem block if it doesn't exist (i.e. when dumping a global variable out of the scope of a function compilation). The patch is at: http://web.ist.utl.pt/nuno.lopes/llvm_jit_global_emitter2.txt
The problem only happens when calling ExecutionEngine::getPointerToGlobal(someGV) from some non-llvm program. If the function is called when JITing a function, it works, since it will dump the global variable to the memory reserved to the function being JITed, which raises a question: shouldn't it generate the GV to some non-executable memory block?? My patch doesn't attempt to fix this last concern.
Thanks,
Nuno
P.S.: the control flow of this bug is quite complex, so feel free to ask if you don't get what the problem is.
[resending since the previous copy was apparently dropped by the mailing list]
Today I found a nice bug in the JIT global variable emitter.
The problem may lead to an assert() failure when doing the following:
1) compile some function
2) emit a global variable
3) compile another function. an assert() may trigger in the JIT memory
manager
This happens because the JIT global variable emitter is using the
MachineCodeEmitter::allocate() function, which uses memory allocated
by the
JIT memory manager (which should be used for functions only).
No, this was a deliberate change, 54442. We have a situation where a
user
wants to emit JIT code on one machine, then send it off to another
machine to
execute. Putting statically allocated data in the same buffer as code
is the
easiest approach to make this work, although there may be others.
Ok, thanks for the explanation. So my first patch doesn't work. Also, to be
clear, this bug has nothing to do with overflowing the JIT memory buffer.
I made another one that takes keeps the allocation of global variables in
the JIT buffer, but it creates a new mem block if it doesn't exist (i.e.
when dumping a global variable out of the scope of a function compilation).
The patch is at: http://web.ist.utl.pt/nuno.lopes/llvm_jit_global_emitter2.txt
The problem only happens when calling
ExecutionEngine::getPointerToGlobal(someGV) from some non-llvm program. If
the function is called when JITing a function, it works, since it will dump
the global variable to the memory reserved to the function being JITed,
which raises a question: shouldn't it generate the GV to some non-executable
memory block?? My patch doesn't attempt to fix this last concern.
Thanks,
Nuno
P.S.: the control flow of this bug is quite complex, so feel free to ask if
you don't get what the problem is.
[resending since the previous copy was apparently dropped by the mailing
list]
Today I found a nice bug in the JIT global variable emitter.
The problem may lead to an assert() failure when doing the following:
1) compile some function
2) emit a global variable
3) compile another function. an assert() may trigger in the JIT memory
manager
This happens because the JIT global variable emitter is using the
MachineCodeEmitter::allocate() function, which uses memory allocated
by the
JIT memory manager (which should be used for functions only).
No, this was a deliberate change, 54442. We have a situation where a
user
wants to emit JIT code on one machine, then send it off to another
machine to
execute. Putting statically allocated data in the same buffer as code
is the
easiest approach to make this work, although there may be others.
Ok, thanks for the explanation. So my first patch doesn't work. Also, to be
clear, this bug has nothing to do with overflowing the JIT memory buffer.
I made another one that takes keeps the allocation of global variables in
the JIT buffer, but it creates a new mem block if it doesn't exist (i.e.
when dumping a global variable out of the scope of a function compilation).
The patch is at: http://web.ist.utl.pt/nuno.lopes/llvm_jit_global_emitter2.txt
Sorry, I am still not able to understand the problem. Is there a bug in the default memory manager? From your patch it seems like there is a real bug. What is the assertion that you ran into?
The problem only happens when calling
ExecutionEngine::getPointerToGlobal(someGV) from some non-llvm program. If
the function is called when JITing a function, it works, since it will dump
the global variable to the memory reserved to the function being JITed,
which raises a question: shouldn't it generate the GV to some non-executable
memory block?? My patch doesn't attempt to fix this last concern.
There are two ways to go about this. Either we enhance memory manager interface to create non-executable memory for GVs etc. Or we keep the memory manager simple and let the JIT change the privilege as it sees fit. I prefer the later unless there is a good argument for an alternative.
Today I found a nice bug in the JIT global variable emitter.
The problem may lead to an assert() failure when doing the
following:
1) compile some function
2) emit a global variable
3) compile another function. an assert() may trigger in the JIT
memory
manager
This happens because the JIT global variable emitter is using the
MachineCodeEmitter::allocate() function, which uses memory allocated
by the
JIT memory manager (which should be used for functions only).
No, this was a deliberate change, 54442. We have a situation where a
user
wants to emit JIT code on one machine, then send it off to another
machine to
execute. Putting statically allocated data in the same buffer as
code
is the
easiest approach to make this work, although there may be others.
Ok, thanks for the explanation. So my first patch doesn't work.
Also, to be
clear, this bug has nothing to do with overflowing the JIT memory
buffer.
I made another one that takes keeps the allocation of global
variables in
the JIT buffer, but it creates a new mem block if it doesn't exist
(i.e.
when dumping a global variable out of the scope of a function
compilation).
The patch is at: http://web.ist.utl.pt/nuno.lopes/llvm_jit_global_emitter2.txt
Sorry, I am still not able to understand the problem. Is there a bug
in the default memory manager? From your patch it seems like there is
a real bug. What is the assertion that you ran into?
This bug is a problem in the design of the JIT GV emitter, that appeared with the revision 54442, as Dale pointed.
I was trying to avoid to make a test case, but I did one now. Please find it in attach (I hope the ML doesn't strip it). The source code has enough comments to explain what's going on.
The module.bc is just a container to have 2 functions, but it's irrelevant to the problem. I just picked some random .bc file that had 2 functions.
The problem only happens when calling
ExecutionEngine::getPointerToGlobal(someGV) from some non-llvm
program. If
the function is called when JITing a function, it works, since it
will dump
the global variable to the memory reserved to the function being
JITed,
which raises a question: shouldn't it generate the GV to some non-
executable
memory block?? My patch doesn't attempt to fix this last concern.
There are two ways to go about this. Either we enhance memory manager
interface to create non-executable memory for GVs etc. Or we keep the
memory manager simple and let the JIT change the privilege as it sees
fit. I prefer the later unless there is a good argument for an
alternative.
I didn't check, but I think the JIT engine is mapping the whole function's memory as executable. As now most global variables end up being created when JITing functions, those GV end up in executable memory.
Today I found a nice bug in the JIT global variable emitter.
The problem may lead to an assert() failure when doing the following:
1) compile some function
2) emit a global variable
3) compile another function. an assert() may trigger in the JIT memory
manager
This happens because the JIT global variable emitter is using the
MachineCodeEmitter::allocate() function, which uses memory allocated
by the
JIT memory manager (which should be used for functions only).
No, this was a deliberate change, 54442. We have a situation where a
user
wants to emit JIT code on one machine, then send it off to another
machine to
execute. Putting statically allocated data in the same buffer as code
is the
easiest approach to make this work, although there may be others.
Ok, thanks for the explanation. So my first patch doesn't work. Also, to be
clear, this bug has nothing to do with overflowing the JIT memory buffer.
I made another one that takes keeps the allocation of global variables in
the JIT buffer, but it creates a new mem block if it doesn't exist (i.e.
when dumping a global variable out of the scope of a function compilation).
The patch is at: http://web.ist.utl.pt/nuno.lopes/llvm_jit_global_emitter2.txt
OK, I think I finally understand this: the allocation in JIT::getOrEmitGlobalVariable doesn't call into JITEmitter but uses the MachineCodeEmitter directly. Thus, if you call it from outside the JITEmitter, it won't update the data structures the JITEmitter relies on. In the usual case, the JITEmitter calls into JIT and updates its own data structures, so that case works fine.
I think adding a smarter allocateSpace to JITEmitter is the right general idea, and your patch looks OK. I'd like to get another pair of eyes on it though, as I find this area not straightfoward:) Please run the testsuite before committing.
Ok, thanks for the explanation. So my first patch doesn't work.
Also, to be
clear, this bug has nothing to do with overflowing the JIT memory
buffer.
I made another one that takes keeps the allocation of global
variables in
the JIT buffer, but it creates a new mem block if it doesn't exist
(i.e.
when dumping a global variable out of the scope of a function
compilation).
The patch is at: http://web.ist.utl.pt/nuno.lopes/llvm_jit_global_emitter2.txt
OK, I think I finally understand this: the allocation in
JIT::getOrEmitGlobalVariable doesn't call into JITEmitter but uses the
MachineCodeEmitter directly. Thus, if you call it from outside the
JITEmitter, it won't update the data structures the JITEmitter relies
on. In the usual case, the JITEmitter calls into JIT and updates its
own data structures, so that case works fine.
exactly! Thank you for summarizing. I hope others can now follow this description.
I think adding a smarter allocateSpace to JITEmitter is the right
general idea, and your patch looks OK. I'd like to get another pair
of eyes on it though, as I find this area not straightfoward:) Please
run the testsuite before committing.
Yes, I would also appreciate more reviewers. The JIT's code is particularly complex and I'm also quite new to the LLVM code..
+ /// allocateSpace - reserves space in the current block if any, or
+ /// allocate a new one of the given size + virtual void
*allocateSpace(intptr_t Size, unsigned Alignment); +
Please capitalize "reserves".
ok.
+ /// allocateSpace - general-purpose space allocator
Better comments please. Also please end the sentence with a period
or Chris' head will explode.
ok, sure, I guess we don't want that
unsigned char *result = (unsigned char *)CurBlock+1;
+
+ if (Alignment == 0) Alignment = 1;
+ result = (unsigned char*)(((intptr_t)result+Alignment-1) &
+ ~(intptr_t)(Alignment-1));
If type of result is intptr_t, you can avoid some of the casting, right?
Well, I basically copied that code from the MachineCodeEmitter class
I must confess that I don't know what the standard says about casting. In the last times I tried guessing (when implementing some parts in clang) I was wrong, so..
Apart of fixing these minor things, may I commit the patch?
+ /// allocateSpace - reserves space in the current block if any, or
+ /// allocate a new one of the given size + virtual void
*allocateSpace(intptr_t Size, unsigned Alignment); +
Please capitalize "reserves".
ok.
+ /// allocateSpace - general-purpose space allocator
Better comments please. Also please end the sentence with a period
or Chris' head will explode.
ok, sure, I guess we don't want that
unsigned char *result = (unsigned char *)CurBlock+1;
+
+ if (Alignment == 0) Alignment = 1;
+ result = (unsigned char*)(((intptr_t)result+Alignment-1) &
+ ~(intptr_t)(Alignment-1));
If type of result is intptr_t, you can avoid some of the casting, right?
Well, I basically copied that code from the MachineCodeEmitter class
I must confess that I don't know what the standard says about casting. In
the last times I tried guessing (when implementing some parts in clang) I
was wrong, so..
Apart of fixing these minor things, may I commit the patch?
Yes! But please do some sanity testing using the test suite.
+ /// allocateSpace - reserves space in the current block if any, or
+ /// allocate a new one of the given size + virtual void
*allocateSpace(intptr_t Size, unsigned Alignment); +
Please capitalize "reserves".
ok.
+ /// allocateSpace - general-purpose space allocator
Better comments please. Also please end the sentence with a
period
or Chris' head will explode.
ok, sure, I guess we don't want that
unsigned char *result = (unsigned char *)CurBlock+1;
+
+ if (Alignment == 0) Alignment = 1;
+ result = (unsigned char*)(((intptr_t)result+Alignment-1) &
+ ~(intptr_t)(Alignment-1));
If type of result is intptr_t, you can avoid some of the casting,
right?
Well, I basically copied that code from the MachineCodeEmitter
class
I must confess that I don't know what the standard says about
casting. In
the last times I tried guessing (when implementing some parts in
clang) I
was wrong, so..
Apart of fixing these minor things, may I commit the patch?
Yes! But please do some sanity testing using the test suite.
Ok, patch commited!
There are no test regressions and the PHP JIT compiler seems to be working again
Thank you for summarizing. I hope others can now follow this description.