Here is the corrected version. Thomas Jablin wrote:> Actually, there is a problem with the patch. Please delay review. > > Thomas Jablin wrote: > >> Hi, >> The following code: >> >> #include<stdio.h> >> >> char bigArray[0x1000000]; >> >> int main(int argc, char **argv) { >> printf("mem: 0x%x\n", (unsigned) bigArray); >> return 0; >> } >> >> causes lli to silently fail, even though it compiles correctly with >> llc. The reason is that in JITEmitter.cpp only checks to see if >> CurBufferPtr == BufferEnd at the beginning of the function and not >> after all relocations have been handled. I have fixed this bug by >> adding an additional check after all relocations have been completed. >> In the process of fixing this bug, I happened to look through the code >> in MachineCodeEmitter.h. The buffer size checks in >> MachineCodeEmitter.h all suffer from an integer overflow bug. For >> example in allocateSpace the code reads: >> >> // Allocate the space. >> CurBufferPtr += Size; >> >> // Check for buffer overflow. >> if (CurBufferPtr >= BufferEnd) { >> >> This is wrong because Size + CurBufferPtr can cause an integer >> overflow and thus appear to be less than BufferEnd. The correct way >> to check for the end of a buffer is always: >> >> (Size >= BufferEnd-CurBufferPtr) >> >> This integer overflow bug causes the program: >> #include<stdio.h> >> >> char b = 'b'; >> char c[0x8000000]; >> >> int main(int argc, char **argv) { >> printf("%c\n", c[0]); >> return 0; >> } >> to segfault in lli. >> >> Finally, I have changed several instances of intptr_t to uintptr_t to >> avoid dangerous comparisons between signed and unsigned types. Code >> review of the enclosed patch would be greatly appreciated. Thanks >> Tom >> ------------------------------------------------------------------------ >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- A non-text attachment was scrubbed... Name: FixEmitter.diff Type: text/x-patch Size: 13801 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081122/6a6311f5/attachment.bin>
Thanks. But we need to match the type changes in all the target. e.g. ARMJITInfo.cpp, X86CodeEmitter.cpp. Also in MachineRelocation, e.g. getBB. Could you prepare a patch with all those fixed as well? Evan On Nov 22, 2008, at 1:19 PM, Thomas Jablin wrote:> Here is the corrected version. > > Thomas Jablin wrote: >> Actually, there is a problem with the patch. Please delay review. >> >> Thomas Jablin wrote: >> >>> Hi, >>> The following code: >>> >>> #include<stdio.h> >>> >>> char bigArray[0x1000000]; >>> >>> int main(int argc, char **argv) { >>> printf("mem: 0x%x\n", (unsigned) bigArray); >>> return 0; >>> } >>> >>> causes lli to silently fail, even though it compiles correctly >>> with llc. The reason is that in JITEmitter.cpp only checks to see >>> if CurBufferPtr == BufferEnd at the beginning of the function and >>> not after all relocations have been handled. I have fixed this bug >>> by adding an additional check after all relocations have been >>> completed. In the process of fixing this bug, I happened to look >>> through the code in MachineCodeEmitter.h. The buffer size checks >>> in MachineCodeEmitter.h all suffer from an integer overflow bug. >>> For example in allocateSpace the code reads: >>> >>> // Allocate the space. >>> CurBufferPtr += Size; >>> // Check for buffer overflow. >>> if (CurBufferPtr >= BufferEnd) { >>> >>> This is wrong because Size + CurBufferPtr can cause an integer >>> overflow and thus appear to be less than BufferEnd. The correct >>> way to check for the end of a buffer is always: >>> >>> (Size >= BufferEnd-CurBufferPtr) >>> >>> This integer overflow bug causes the program: >>> #include<stdio.h> >>> >>> char b = 'b'; >>> char c[0x8000000]; >>> >>> int main(int argc, char **argv) { >>> printf("%c\n", c[0]); >>> return 0; >>> } >>> to segfault in lli. >>> >>> Finally, I have changed several instances of intptr_t to uintptr_t >>> to avoid dangerous comparisons between signed and unsigned types. >>> Code review of the enclosed patch would be greatly appreciated. >>> Thanks >>> Tom >>> ------------------------------------------------------------------------ >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > <FixEmitter.diff>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Looks good. Do you have commit privilege? Evan On Nov 22, 2008, at 1:19 PM, Thomas Jablin wrote:> Here is the corrected version. > > Thomas Jablin wrote: >> Actually, there is a problem with the patch. Please delay review. >> >> Thomas Jablin wrote: >> >>> Hi, >>> The following code: >>> >>> #include<stdio.h> >>> >>> char bigArray[0x1000000]; >>> >>> int main(int argc, char **argv) { >>> printf("mem: 0x%x\n", (unsigned) bigArray); >>> return 0; >>> } >>> >>> causes lli to silently fail, even though it compiles correctly >>> with llc. The reason is that in JITEmitter.cpp only checks to see >>> if CurBufferPtr == BufferEnd at the beginning of the function and >>> not after all relocations have been handled. I have fixed this bug >>> by adding an additional check after all relocations have been >>> completed. In the process of fixing this bug, I happened to look >>> through the code in MachineCodeEmitter.h. The buffer size checks >>> in MachineCodeEmitter.h all suffer from an integer overflow bug. >>> For example in allocateSpace the code reads: >>> >>> // Allocate the space. >>> CurBufferPtr += Size; >>> // Check for buffer overflow. >>> if (CurBufferPtr >= BufferEnd) { >>> >>> This is wrong because Size + CurBufferPtr can cause an integer >>> overflow and thus appear to be less than BufferEnd. The correct >>> way to check for the end of a buffer is always: >>> >>> (Size >= BufferEnd-CurBufferPtr) >>> >>> This integer overflow bug causes the program: >>> #include<stdio.h> >>> >>> char b = 'b'; >>> char c[0x8000000]; >>> >>> int main(int argc, char **argv) { >>> printf("%c\n", c[0]); >>> return 0; >>> } >>> to segfault in lli. >>> >>> Finally, I have changed several instances of intptr_t to uintptr_t >>> to avoid dangerous comparisons between signed and unsigned types. >>> Code review of the enclosed patch would be greatly appreciated. >>> Thanks >>> Tom >>> ------------------------------------------------------------------------ >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > <FixEmitter.diff>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Thanks. I do not have commit privilege. Tom ----- Original Message ----- From: "Evan Cheng" <evan.cheng at apple.com> To: "LLVM Developers Mailing List" <llvmdev at cs.uiuc.edu> Sent: Monday, December 8, 2008 5:39:33 PM GMT -05:00 US/Canada Eastern Subject: Re: [LLVMdev] MachineCodeEmitter Patch Looks good. Do you have commit privilege? Evan On Nov 22, 2008, at 1:19 PM, Thomas Jablin wrote:> Here is the corrected version. > > Thomas Jablin wrote: >> Actually, there is a problem with the patch. Please delay review. >> >> Thomas Jablin wrote: >> >>> Hi, >>> The following code: >>> >>> #include<stdio.h> >>> >>> char bigArray[0x1000000]; >>> >>> int main(int argc, char **argv) { >>> printf("mem: 0x%x\n", (unsigned) bigArray); >>> return 0; >>> } >>> >>> causes lli to silently fail, even though it compiles correctly >>> with llc. The reason is that in JITEmitter.cpp only checks to see >>> if CurBufferPtr == BufferEnd at the beginning of the function and >>> not after all relocations have been handled. I have fixed this bug >>> by adding an additional check after all relocations have been >>> completed. In the process of fixing this bug, I happened to look >>> through the code in MachineCodeEmitter.h. The buffer size checks >>> in MachineCodeEmitter.h all suffer from an integer overflow bug. >>> For example in allocateSpace the code reads: >>> >>> // Allocate the space. >>> CurBufferPtr += Size; >>> // Check for buffer overflow. >>> if (CurBufferPtr >= BufferEnd) { >>> >>> This is wrong because Size + CurBufferPtr can cause an integer >>> overflow and thus appear to be less than BufferEnd. The correct >>> way to check for the end of a buffer is always: >>> >>> (Size >= BufferEnd-CurBufferPtr) >>> >>> This integer overflow bug causes the program: >>> #include<stdio.h> >>> >>> char b = 'b'; >>> char c[0x8000000]; >>> >>> int main(int argc, char **argv) { >>> printf("%c\n", c[0]); >>> return 0; >>> } >>> to segfault in lli. >>> >>> Finally, I have changed several instances of intptr_t to uintptr_t >>> to avoid dangerous comparisons between signed and unsigned types. >>> Code review of the enclosed patch would be greatly appreciated. >>> Thanks >>> Tom >>> ------------------------------------------------------------------------ >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > <FixEmitter.diff>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev_______________________________________________ LLVM Developers mailing list LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev