Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14617017:
Adding a pass - RewritePNaClLibraryCalls, that replaces known library calls with stable bitcode int… (Closed)
|
Created:
7 years, 7 months ago by eliben Modified:
7 years, 7 months ago CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master Visibility:
Public. |
DescriptionAdding a pass - RewritePNaClLibraryCalls, that replaces known library calls with stable bitcode intrinsics.
Starting with setjmp and longjmp.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3429
R=jvoung@chromium.org, mseaborn@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=4a71e62
Patch Set 1 #
Total comments: 23
Patch Set 2 : Adding handling of address taking of longjmp in stores and globals #Patch Set 3 : Adding forgotten test #
Total comments: 15
Patch Set 4 : Fixing for Jan's comments #
Total comments: 17
Patch Set 5 : Populate the body of longjmp if it has non-call uses #Patch Set 6 : Added llvm.nacl.{set|long}jmp to the list of whitelisted intrinsics for ABI #
Total comments: 23
Patch Set 7 : Addressed latest review comments #
Total comments: 1
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:10: // This pass replaces calls to known library functions by calls to intrinsics 'by' -> 'with' https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:25: class RewritePNaClLibraryCalls : public ModulePass { Maybe comment why this is a ModulePass? Does it need to be a ModulePass if it introduces declarations for intrinsics? https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:52: for (Module::iterator MI = M.begin(), ME = M.end(); MI != ME; ++MI) { Iterating through all calls is inefficient. It would be better to get the setjmp/longjmp globals and iterate through their uses. You should also check that the functions aren't address-taken... It's not legal for setjmp() to be address-taken: "It is unspecified whether setjmp() is a macro or a function." (http://pubs.opengroup.org/onlinepubs/009696799/functions/setjmp.html) However, I think it's legal to take the address of longjmp() since it's described as a function (http://pubs.opengroup.org/onlinepubs/009696799/functions/longjmp.html). Maybe we could handle that case by creating a function that wraps the longjmp intrinsic? https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:58: StringRef CalleeName = Call->getCalledFunction()->getName(); I think you need to check the linkage type too. These could be internal functions called "setjmp" or "longjmp" (maybe static) if we haven't stripped the internal names yet. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:88: Type *I8Ptr = Type::getInt8Ty(TheModule->getContext())->getPointerTo(); You could use: NaclSetjmpFunc->getFunctionType()->getParamType(0) to avoid fixing the knowledge of the arg type being i8* here. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:94: Function *NaclSetjmpFunc = Intrinsic::getDeclaration(TheModule, Maybe "Nacl" -> "NaCl"? https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:99: NaclSetjmpCall->setDebugLoc(DLoc); Use takeName() too https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:126: Intrinsic::nacl_longjmp); Line >80 chars https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... File test/Transforms/NaCl/rewrite-setlongjmp-calls.ll (right): https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:10: ; CHECK-NEXT: call i32 @llvm.nacl.setjmp(i8* %jmp_buf_i8) Make this: "%val = call"
https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... File test/Transforms/NaCl/rewrite-setlongjmp-calls.ll (right): https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:4: declare i32 @setjmp(i64*) CHECK that it now declares the llvm.nacl.setjmp and longjmp, as well? https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:29: call void @longjmp(i64* %arg, i32 %num) This function doesn't quite have the "normal" usage of longjmp and setjmp. longjmp doesn't return to the point right after it (almost like there's an "unreachable" inst right after), so it's a bit weird to be doing a bunch of stuff after. What are the important parts of this test case? That the rewrite traverses all blocks of the function? That you properly replace all uses of the setjmp return value?
https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:10: // This pass replaces calls to known library functions by calls to intrinsics On 2013/05/10 16:34:36, Mark Seaborn wrote: > 'by' -> 'with' Done. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:25: class RewritePNaClLibraryCalls : public ModulePass { On 2013/05/10 16:34:36, Mark Seaborn wrote: > Maybe comment why this is a ModulePass? Does it need to be a ModulePass if it > introduces declarations for intrinsics? Done. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:52: for (Module::iterator MI = M.begin(), ME = M.end(); MI != ME; ++MI) { On 2013/05/10 16:34:36, Mark Seaborn wrote: > Iterating through all calls is inefficient. It would be better to get the > setjmp/longjmp globals and iterate through their uses. > Yeah, although given a larger amount of functions it will likely make the code more complex. I'll experiment to see if it's not too bad. > You should also check that the functions aren't address-taken... > Good point. > It's not legal for setjmp() to be address-taken: "It is unspecified whether > setjmp() is a macro or a function." > (http://pubs.opengroup.org/onlinepubs/009696799/functions/setjmp.html) > Yes, the C++11 standard explicitly says: " Names which are defined as macros in C shall be defined as macros in the C++ standard library, even if C grants license for implementation as functions. [ Note: The names defined as macros in C include the following: assert, offsetof, setjmp, va_arg, va_end, and va_start. — end note ]" > However, I think it's legal to take the address of longjmp() since it's > described as a function > (http://pubs.opengroup.org/onlinepubs/009696799/functions/longjmp.html). > > Maybe we could handle that case by creating a function that wraps the longjmp > intrinsic? Where should this function be created? I wonder if we can just write it in IR somewhere in pnacl/support/bitcode and then it will just do the cast and call the intrinsic. The pass will then just have to replace one call by another.
... will upload updated code later https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:52: for (Module::iterator MI = M.begin(), ME = M.end(); MI != ME; ++MI) { On 2013/05/10 16:34:36, Mark Seaborn wrote: > Iterating through all calls is inefficient. It would be better to get the > setjmp/longjmp globals and iterate through their uses. > > You should also check that the functions aren't address-taken... > > It's not legal for setjmp() to be address-taken: "It is unspecified whether > setjmp() is a macro or a function." > (http://pubs.opengroup.org/onlinepubs/009696799/functions/setjmp.html) > > However, I think it's legal to take the address of longjmp() since it's > described as a function > (http://pubs.opengroup.org/onlinepubs/009696799/functions/longjmp.html). > > Maybe we could handle that case by creating a function that wraps the longjmp > intrinsic? Done. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:58: StringRef CalleeName = Call->getCalledFunction()->getName(); On 2013/05/10 16:34:36, Mark Seaborn wrote: > I think you need to check the linkage type too. These could be internal > functions called "setjmp" or "longjmp" (maybe static) if we haven't stripped the > internal names yet. Done. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:94: Function *NaclSetjmpFunc = Intrinsic::getDeclaration(TheModule, On 2013/05/10 16:34:36, Mark Seaborn wrote: > Maybe "Nacl" -> "NaCl"? Done. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:99: NaclSetjmpCall->setDebugLoc(DLoc); On 2013/05/10 16:34:36, Mark Seaborn wrote: > Use takeName() too Done. https://codereview.chromium.org/14617017/diff/1/lib/Transforms/NaCl/RewritePN... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:126: Intrinsic::nacl_longjmp); On 2013/05/10 16:34:36, Mark Seaborn wrote: > Line >80 chars Done. https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... File test/Transforms/NaCl/rewrite-setlongjmp-calls.ll (right): https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:4: declare i32 @setjmp(i64*) On 2013/05/10 16:42:49, jvoung (cr) wrote: > CHECK that it now declares the llvm.nacl.setjmp and longjmp, as well? Done. https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:4: declare i32 @setjmp(i64*) On 2013/05/10 16:42:49, jvoung (cr) wrote: > CHECK that it now declares the llvm.nacl.setjmp and longjmp, as well? Done. https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:10: ; CHECK-NEXT: call i32 @llvm.nacl.setjmp(i8* %jmp_buf_i8) On 2013/05/10 16:34:36, Mark Seaborn wrote: > Make this: "%val = call" Done. https://codereview.chromium.org/14617017/diff/1/test/Transforms/NaCl/rewrite-... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:29: call void @longjmp(i64* %arg, i32 %num) On 2013/05/10 16:42:49, jvoung (cr) wrote: > This function doesn't quite have the "normal" usage of longjmp and setjmp. > > longjmp doesn't return to the point right after it (almost like there's an > "unreachable" inst right after), so it's a bit weird to be doing a bunch of > stuff after. > > What are the important parts of this test case? That the rewrite traverses all > blocks of the function? That you properly replace all uses of the setjmp return > value? Yes, these tests (and the pass, really) have nothing to do with the actual semantics of setjmp/longjmp. But since it does a bunch of replacements I figured it would make sense to test it on a non-trivial "single BB with just the right function appearing once" case. It helps code coverage ;-)
I've added handling of longjmp's address being stored in instructions or globals, by creating a wrapper function that gets its address taken instead. This wrapper does the bitcast and intrinsic call.
https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:35: // intrindic declarations into the module. intrindic -> intrinsic Another reason (if you want to add to the comment) is that it may need to rewrite global intializers that take the address of longjmp. https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:36: initializeExpandTlsPass(*PassRegistry::getPassRegistry()); initializeExpandTlsPass? https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:61: Function *SetjmpFunc; Does SetjmpFunc really need to be a field? It looks like it's only used in the runOnModule function. LongjmpFunc could be passed along from runOnModule to the rewrite functions as an argument, so it doesn't really need to be a field either. Then you wouldn't need to initialize these to 0. The LongjmpWrapperFunc makes sense, since that is state from the getOrCreate... method. https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:85: // For longjmp calls are also replaced; however, longjmp can have its no need for two spaces between ";" and "however"? https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:95: rewriteSetjmpCall(Call); Now that you aren't checking "Call->getCalledFunction()->getName();", should it check? Is it possible to have a use of setjmp as a function parameter? E.g., if you have "printf("my setjmp is at addr: %p\n", &setjmp);" https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:99: } Do we expect to see other uses of setjmp (else branch -> error)? Same with longjmp (else branch -> error)? https://codereview.chromium.org/14617017/diff/10001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-setjmp-store-error.ll (right): https://codereview.chromium.org/14617017/diff/10001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-setjmp-store-error.ll:9: ; CHECK: Invalid store of setjmp How about "Error: cannot take address of setjmp" or something more related to what is happening in the C source?
https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:35: // intrindic declarations into the module. On 2013/05/13 18:12:30, jvoung (cr) wrote: > intrindic -> intrinsic > > Another reason (if you want to add to the comment) is that it may need to > rewrite global intializers that take the address of longjmp. Done. https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:36: initializeExpandTlsPass(*PassRegistry::getPassRegistry()); On 2013/05/13 18:12:30, jvoung (cr) wrote: > initializeExpandTlsPass? Oops :-/ Thanks, fixed. https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:61: Function *SetjmpFunc; On 2013/05/13 18:12:30, jvoung (cr) wrote: > Does SetjmpFunc really need to be a field? It looks like it's only used in the > runOnModule function. > > LongjmpFunc could be passed along from runOnModule to the rewrite functions as > an argument, so it doesn't really need to be a field either. > > Then you wouldn't need to initialize these to 0. > > The LongjmpWrapperFunc makes sense, since that is state from the getOrCreate... > method. Yeah, SetjmpFunc is definitely a leftover from refactoring. I'll remove it. LongjmpFunc though is used in getOrCreateLongjmpWrapper too so I don't know if it's worth piping through a bunch of places. Do you think that would be better? https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:85: // For longjmp calls are also replaced; however, longjmp can have its On 2013/05/13 18:12:30, jvoung (cr) wrote: > no need for two spaces between ";" and "however"? Done. https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:95: rewriteSetjmpCall(Call); On 2013/05/13 18:12:30, jvoung (cr) wrote: > Now that you aren't checking "Call->getCalledFunction()->getName();", should it > check? > Not sure what you mean. Only going over the uses of setjmp here. > Is it possible to have a use of setjmp as a function parameter? E.g., if you > have "printf("my setjmp is at addr: %p\n", &setjmp);" No, it's illegal to take the address of setjmp in any way https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:99: } On 2013/05/13 18:12:30, jvoung (cr) wrote: > Do we expect to see other uses of setjmp (else branch -> error)? > > Same with longjmp (else branch -> error)? Done. https://codereview.chromium.org/14617017/diff/10001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-setjmp-store-error.ll (right): https://codereview.chromium.org/14617017/diff/10001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-setjmp-store-error.ll:9: ; CHECK: Invalid store of setjmp On 2013/05/13 18:12:30, jvoung (cr) wrote: > How about "Error: cannot take address of setjmp" or something more related to > what is happening in the C source? Done.
LGTM https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/10001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:95: rewriteSetjmpCall(Call); On 2013/05/13 18:33:36, eliben wrote: > On 2013/05/13 18:12:30, jvoung (cr) wrote: > > Now that you aren't checking "Call->getCalledFunction()->getName();", should > it > > check? > > > > Not sure what you mean. Only going over the uses of setjmp here. > > > Is it possible to have a use of setjmp as a function parameter? E.g., if you > > have "printf("my setjmp is at addr: %p\n", &setjmp);" > > No, it's illegal to take the address of setjmp in any way > I was wondering if you had IL like this: %call = call i32 (i8*, ...)* @printf(i8* getelementptr inbounds ([4 x i8]* @.str, i32 0, i32 0), i32 (i8*)* @setjmp) Would that be considered a "user" of setjmp, within a CallInst, even though setjmp is an operand, and not the called function. I guess not, because it's only an operand, and not a value defined by a call to setjmp? It seems like you are also relying on the ABI checker rejecting constant exprs (and the const expr expansion pass running), otherwise the address could have been taken in arbitrary places (like the above IL). https://codereview.chromium.org/14617017/diff/19001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-global-ptr-longjmp.ll (right): https://codereview.chromium.org/14617017/diff/19001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-global-ptr-longjmp.ll:10: ; CHECK: define private void @__nacl_wrap_longjmp(i64* %env, i32 %val) { reduce the amount of spacing between the CHECK: and the content?
https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:32: ModulePass(ID), TheModule(0), Indent by 2 more spaces so that this is more indented than the method body https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:33: LongjmpFunc(0), LongjmpWrapperFunc(0) { 0 -> NULL? https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:48: /// Rewrites the given \p Store instruction that stores a pointer to Why are you checking specifically for Store instructions? What about other uses, e.g. ptrtoint? https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:52: void rewriteAddrOfLongjmpInGlobal(GlobalVariable *Global); This also doesn't make sense. You should handle all uses generically, with replacesAllUsesWith().
https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:150: SmallVector<Value *, 2> Args; Nit: 1 will do here instead of 2 https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:207: // Replace Store by a new store that stores a pointer to the wrapper. You should bitcast your wrapper function rather than recreating store instructions and globals with a different type. You can use a bitcast ConstantExpr (this pass can run before ExpandConstantExpr). Actually, you don't even need a bitcast, since your wrapper function has the same type as the original longjmp(). See comment below... https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:247: // A wrapper function is created that has the same type as longjmp; it You actually don't need to create a new function. A function declaration is just a function with no basic blocks. You can just insert a basic block into the existing longjmp() function. Then your wrapper will still be named longjmp() (without a takeName() call). You might need to change the linkage to be internal. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:250: SmallString<32> Name(WRAP_FUNC_PREFIX); Name.append(LONGJMP_NAME); Split onto separate lines. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:265: // Insert a return instruction Nit: could insert 'unreachable' instead
PTAL. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:32: ModulePass(ID), TheModule(0), On 2013/05/13 20:09:00, Mark Seaborn wrote: > Indent by 2 more spaces so that this is more indented than the method body Done. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:33: LongjmpFunc(0), LongjmpWrapperFunc(0) { On 2013/05/13 20:09:00, Mark Seaborn wrote: > 0 -> NULL? Done. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:150: SmallVector<Value *, 2> Args; On 2013/05/13 20:20:42, Mark Seaborn wrote: > Nit: 1 will do here instead of 2 Done. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:247: // A wrapper function is created that has the same type as longjmp; it On 2013/05/13 20:20:42, Mark Seaborn wrote: > You actually don't need to create a new function. > > A function declaration is just a function with no basic blocks. You can just > insert a basic block into the existing longjmp() function. > > Then your wrapper will still be named longjmp() (without a takeName() call). > You might need to change the linkage to be internal. Done. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:250: SmallString<32> Name(WRAP_FUNC_PREFIX); Name.append(LONGJMP_NAME); On 2013/05/13 20:20:42, Mark Seaborn wrote: > Split onto separate lines. Function removed. https://codereview.chromium.org/14617017/diff/19001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:265: // Insert a return instruction On 2013/05/13 20:20:42, Mark Seaborn wrote: > Nit: could insert 'unreachable' instead Done. https://codereview.chromium.org/14617017/diff/19001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-global-ptr-longjmp.ll (right): https://codereview.chromium.org/14617017/diff/19001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-global-ptr-longjmp.ll:10: ; CHECK: define private void @__nacl_wrap_longjmp(i64* %env, i32 %val) { On 2013/05/13 19:52:07, jvoung (cr) wrote: > reduce the amount of spacing between the CHECK: and the content? Done.
LGTM, but it looks like rewrite-longjmp-*.ll should be removed in favour of rewrite-setlongjmp-calls.ll, or vice-versa. https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:53: static const char *LONGJMP_NAME = "longjmp"; I don't think LLVM uses NAMES_IN_CAPS for non-#define constants. Also, this doesn't declare a constant. This would: static const char *const LONGJMP_NAME = "longjmp"; But this is better because it allegedly avoids a relocation: static const char LONGJMP_NAME[] = "longjmp"; https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:71: // of setjmp are disallowed (taking the address of setjmp is disallowed in Re-wrap (partly fits on previous line) https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:109: if (LongjmpHasNoncallUses) { Could just do: if (!LongjmpFunc->use_empty()) then you don't need LongjmpHasNoncallUses, and this can go inside the previous 'if'. https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:193: Function::arg_iterator LongjmpArgs = LongjmpFunc->arg_begin(); Also do a sanity check on this function's args, as you do in rewriteLongjmpCall(). If there are only indirect calls to a zero-arg longjmp(void), I suspect the code below will crash. https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:201: Instruction *End = new UnreachableInst(Context, BB); Nit: It would be a little more obvious to add the instructions in order and add this after you add the CallInst. (So pass BB to CallInst::Create().) https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:223: Nit: don't put empty lines at the end of the file (same for other files) https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-longjmp-no-store.ll (right): https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-no-store.ll:6: ; CHECK-NOT: define internal void @longjmp(i64* %env, i32 %val) { Maybe make this stricter: ; CHECK-NOT: define{{.*}}@longjmp( https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-no-store.ll:10: ; CHECK: call void @llvm.nacl.longjmp(i8* %jmp_buf_i8, i32 %num) Maybe check for the bitcast too? Oh, I see rewrite-setlongjmp-calls.ll checks for the bitcast. https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll (right): https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll:10: ; CHECK: define internal void @longjmp(i64* %env, i32 %val) { Check definition of longjmp() too. https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll:14: ; Function Attrs: nounwind What's this comment for? Remove? https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll:22: declare void @somefunc(i8*) Put declaration before use? https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-setlongjmp-calls.ll (right): https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:25: define void @call_longjmp(i64* %arg, i32 %num) { Seems to duplicate @call_longjmp() in rewrite-longjmp-no-store.ll. Maybe remove one or the other? It's confusing if there's multiple tests covering the same case.
https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... File lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp (right): https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:53: static const char *LONGJMP_NAME = "longjmp"; On 2013/05/13 23:39:58, Mark Seaborn wrote: > I don't think LLVM uses NAMES_IN_CAPS for non-#define constants. > > Also, this doesn't declare a constant. This would: > static const char *const LONGJMP_NAME = "longjmp"; > > But this is better because it allegedly avoids a relocation: > static const char LONGJMP_NAME[] = "longjmp"; These constants are now just used in a single place so I'll remove them as they don't make things clearer. https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:71: // of setjmp are disallowed (taking the address of setjmp is disallowed in On 2013/05/13 23:39:58, Mark Seaborn wrote: > Re-wrap (partly fits on previous line) Done. https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:109: if (LongjmpHasNoncallUses) { On 2013/05/13 23:39:58, Mark Seaborn wrote: > Could just do: > if (!LongjmpFunc->use_empty()) > then you don't need LongjmpHasNoncallUses, and this can go inside the previous > 'if'. Done. https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:193: Function::arg_iterator LongjmpArgs = LongjmpFunc->arg_begin(); On 2013/05/13 23:39:58, Mark Seaborn wrote: > Also do a sanity check on this function's args, as you do in > rewriteLongjmpCall(). If there are only indirect calls to a zero-arg > longjmp(void), I suspect the code below will crash. Refactored https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:201: Instruction *End = new UnreachableInst(Context, BB); On 2013/05/13 23:39:58, Mark Seaborn wrote: > Nit: It would be a little more obvious to add the instructions in order and add > this after you add the CallInst. (So pass BB to CallInst::Create().) Done. https://codereview.chromium.org/14617017/diff/28001/lib/Transforms/NaCl/Rewri... lib/Transforms/NaCl/RewritePNaClLibraryCalls.cpp:223: On 2013/05/13 23:39:58, Mark Seaborn wrote: > Nit: don't put empty lines at the end of the file (same for other files) Done. https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-longjmp-no-store.ll (right): https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-no-store.ll:6: ; CHECK-NOT: define internal void @longjmp(i64* %env, i32 %val) { On 2013/05/13 23:39:58, Mark Seaborn wrote: > Maybe make this stricter: > ; CHECK-NOT: define{{.*}}@longjmp( Done. https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll (right): https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll:10: ; CHECK: define internal void @longjmp(i64* %env, i32 %val) { On 2013/05/13 23:39:58, Mark Seaborn wrote: > Check definition of longjmp() too. Done elsewhere https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll:14: ; Function Attrs: nounwind On 2013/05/13 23:39:58, Mark Seaborn wrote: > What's this comment for? Remove? Done. https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-longjmp-noncall-uses.ll:22: declare void @somefunc(i8*) On 2013/05/13 23:39:58, Mark Seaborn wrote: > Put declaration before use? Done. https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-setlongjmp-calls.ll (right): https://codereview.chromium.org/14617017/diff/28001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-setlongjmp-calls.ll:25: define void @call_longjmp(i64* %arg, i32 %num) { On 2013/05/13 23:39:58, Mark Seaborn wrote: > Seems to duplicate @call_longjmp() in rewrite-longjmp-no-store.ll. Maybe remove > one or the other? It's confusing if there's multiple tests covering the same > case. rewrite-longjmp-no-store.ll is targeted specifically at the case where there are no uses other calls, no body is generated. I've clarified the comment.
Message was sent while issue was closed.
Committed patchset #7 manually as r4a71e62 (presubmit successful).
Message was sent while issue was closed.
LGTM
Message was sent while issue was closed.
https://codereview.chromium.org/14617017/diff/34001/test/Transforms/NaCl/rewr... File test/Transforms/NaCl/rewrite-setjmp-store-error.ll (right): https://codereview.chromium.org/14617017/diff/34001/test/Transforms/NaCl/rewr... test/Transforms/NaCl/rewrite-setjmp-store-error.ll:1: ; RUN: opt < %s -rewrite-pnacl-library-calls -S |& FileCheck %s This syntax fails on Mac OS X: /Volumes/data/b/build/slave/nacl-toolchain/build/native_client/pnacl/build/llvm_x86_64/test/Transforms/NaCl/Output/rewrite-setjmp-store-error.ll.script: line 1: syntax error near unexpected token `&' (from http://build.chromium.org/p/tryserver.nacl/builders/nacl-toolchain-mac-pnacl-...) It seems there are no other tests using the "|&" syntax. Do you need to use "2>&1 |" instead? BTW, I'm now sure how you get 'lit' to ignore the exit status of "opt" for tests like this.
Message was sent while issue was closed.
On 2013/05/16 02:23:38, Mark Seaborn wrote: > https://codereview.chromium.org/14617017/diff/34001/test/Transforms/NaCl/rewr... > File test/Transforms/NaCl/rewrite-setjmp-store-error.ll (right): > > https://codereview.chromium.org/14617017/diff/34001/test/Transforms/NaCl/rewr... > test/Transforms/NaCl/rewrite-setjmp-store-error.ll:1: ; RUN: opt < %s > -rewrite-pnacl-library-calls -S |& FileCheck %s > This syntax fails on Mac OS X: > > /Volumes/data/b/build/slave/nacl-toolchain/build/native_client/pnacl/build/llvm_x86_64/test/Transforms/NaCl/Output/rewrite-setjmp-store-error.ll.script: > line 1: syntax error near unexpected token `&' > > (from > http://build.chromium.org/p/tryserver.nacl/builders/nacl-toolchain-mac-pnacl-...) > > It seems there are no other tests using the "|&" syntax. Do you need to use > "2>&1 |" instead? Right, thanks. Unfortunately I don't have time to really work on this until Monday, so I've removed this test temporarily in b7be947d358b5fef5ff80fe001c1c910ab60b36d and will fix it properly when I get back. > > BTW, I'm now sure how you get 'lit' to ignore the exit status of "opt" for tests > like this.
Message was sent while issue was closed.
On 2013/05/16 02:23:38, Mark Seaborn wrote: > https://codereview.chromium.org/14617017/diff/34001/test/Transforms/NaCl/rewr... > File test/Transforms/NaCl/rewrite-setjmp-store-error.ll (right): > > https://codereview.chromium.org/14617017/diff/34001/test/Transforms/NaCl/rewr... > test/Transforms/NaCl/rewrite-setjmp-store-error.ll:1: ; RUN: opt < %s > -rewrite-pnacl-library-calls -S |& FileCheck %s > This syntax fails on Mac OS X: > > /Volumes/data/b/build/slave/nacl-toolchain/build/native_client/pnacl/build/llvm_x86_64/test/Transforms/NaCl/Output/rewrite-setjmp-store-error.ll.script: > line 1: syntax error near unexpected token `&' > > (from > http://build.chromium.org/p/tryserver.nacl/builders/nacl-toolchain-mac-pnacl-...) > > It seems there are no other tests using the "|&" syntax. Do you need to use > "2>&1 |" instead? > https://codereview.chromium.org/15470008/ > BTW, I'm now sure how you get 'lit' to ignore the exit status of "opt" for tests > like this. I'm fairly sure that lit pays attention only to the exit code of the whole command in this case. It converts the whole pipe into a bash script and runs that. There are other tests in the llvm (and clang) regression suit that do something similar (run a command that returns non-zero, prints an error message and then FileCheck for the error message) |
