|
|
Patch Set 1 #Patch Set 2 : added includes #Patch Set 3 : make it build #
Total comments: 24
Patch Set 4 : style changes #Patch Set 5 : inline the string constant, add comments #Patch Set 6 : style change #Patch Set 7 : rename files to ModuleUtils #Patch Set 8 : comments #
Total messages: 4
|
kcc
Nick, Chris asked me to move one utility function from AddressSanitizer to llvm/Transforms/Utils. Could you ...
|
14 years, 2 months ago (2011年10月28日 19:41:01 UTC) #1 | ||||||||||||||||||||||||||||||||
Nick, Chris asked me to move one utility function from AddressSanitizer to llvm/Transforms/Utils. Could you please check this CL before I send it to the rest of llvmdev? Also, would you suggest how (if at all) test this function? Currently, it is not used outside of asan.
http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... File include/llvm/Transforms/Utils/GlobalCtors.h (right): http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... include/llvm/Transforms/Utils/GlobalCtors.h:22: /// Return the name of the array containing global ctors. It's not immediately obvious to me why this is here, and what problem it solves? http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... include/llvm/Transforms/Utils/GlobalCtors.h:25: /// Append F to the list of global ctors of module M with the given Priority. This needs better comments. What does 'Priority" mean? what is it actually used for? etc. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... File lib/Transforms/Utils/GlobalCtors.cpp (right): http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:21: /// Return the name of the array containing global ctors. There is no need to duplicate comments here. The fact that this just returns a constant value though makes me really question what its goal is.... Why is this name guaranteed? Why doesn't the system that provides that guarantee expose the name in some interface? http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:34: Ty, IRB.getInt32(Priority), F, NULL); Your comment should explain specifically that this routine automates the process of forming a global structure with a constructor that calls the desired function, etc. Essentially, why this is more than a '.appond(F)' operation. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:39: GlobalVariable * GVCtor = M.getNamedGlobal(getGlobalCtorsArrayName()); If this is the only user of getGlobalCtorsArrayName, just inline it here? http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:40: if (GVCtor) { The convention in LLVM is: if (GlobalVariable *GVCtor = M.getNamedGlobal(...)) { ... http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:41: if (Constant *Const = GVCtor->getInitializer()) { I think "Init" is more clear than "Const". http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:42: for (uint32_t index = 0, num = Const->getNumOperands(); More conventional would be: for (unsigned i = 0, e = Const->getNumOperands(); i != e; ++i) ... http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:44: CurrentCtors.push_back(cast<Constant>(Const->getOperand(index))); Can you reserve Const->getNumOperands() + 1 before the for loop? http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:45: } No need for curly here http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:46: } Or here http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:60: new GlobalVariable(M, I wouldn't work so hard to put each argument on its own line. Also I would cast it explicitly to void to indicate that it is intentionally being dropped on the floor.
http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... File include/llvm/Transforms/Utils/GlobalCtors.h (right): http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... include/llvm/Transforms/Utils/GlobalCtors.h:22: /// Return the name of the array containing global ctors. On 2011年10月28日 20:02:15, chandlerc wrote: > It's not immediately obvious to me why this is here, and what problem it solves? Would it be better to just have a global 'const char *' object? Or just use "llvm.global_ctors"? Right now, "llvm.global_ctors" is used in 9 different files as a literal constant http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... include/llvm/Transforms/Utils/GlobalCtors.h:25: /// Append F to the list of global ctors of module M with the given Priority. On 2011年10月28日 20:02:15, chandlerc wrote: > This needs better comments. What does 'Priority" mean? what is it actually used > for? etc. This is documented at http://llvm.org/docs/LangRef.html#intg_global_ctors Shall I give a link? http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... File lib/Transforms/Utils/GlobalCtors.cpp (right): http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:21: /// Return the name of the array containing global ctors. On 2011年10月28日 20:02:15, chandlerc wrote: > There is no need to duplicate comments here. > > The fact that this just returns a constant value though makes me really question > what its goal is.... Why is this name guaranteed? Why doesn't the system that > provides that guarantee expose the name in some interface? Done. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:39: GlobalVariable * GVCtor = M.getNamedGlobal(getGlobalCtorsArrayName()); On 2011年10月28日 20:02:15, chandlerc wrote: > If this is the only user of getGlobalCtorsArrayName, just inline it here? As I mentioned, "llvm.global_ctors" is used in 9 different files. Imo, it would be nice to replace them with a constant or a function call. But not in this cl. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:40: if (GVCtor) { On 2011年10月28日 20:02:15, chandlerc wrote: > The convention in LLVM is: > > if (GlobalVariable *GVCtor = M.getNamedGlobal(...)) { > ... Done. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:41: if (Constant *Const = GVCtor->getInitializer()) { On 2011年10月28日 20:02:15, chandlerc wrote: > I think "Init" is more clear than "Const". Done. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:42: for (uint32_t index = 0, num = Const->getNumOperands(); On 2011年10月28日 20:02:15, chandlerc wrote: > More conventional would be: > > for (unsigned i = 0, e = Const->getNumOperands(); i != e; ++i) > ... Done. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:44: CurrentCtors.push_back(cast<Constant>(Const->getOperand(index))); On 2011年10月28日 20:02:15, chandlerc wrote: > Can you reserve Const->getNumOperands() + 1 before the for loop? Done. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:45: } On 2011年10月28日 20:02:15, chandlerc wrote: > No need for curly here Done. http://codereview.appspot.com/5330046/diff/3001/lib/Transforms/Utils/GlobalCt... lib/Transforms/Utils/GlobalCtors.cpp:60: new GlobalVariable(M, On 2011年10月28日 20:02:15, chandlerc wrote: > I wouldn't work so hard to put each argument on its own line. > > Also I would cast it explicitly to void to indicate that it is intentionally > being dropped on the floor. Done.
Cool, by all means mail upstream. =] Make sure to include some context with the patch so folks know where you'll be using it. http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... File include/llvm/Transforms/Utils/GlobalCtors.h (right): http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... include/llvm/Transforms/Utils/GlobalCtors.h:22: /// Return the name of the array containing global ctors. On 2011年10月28日 20:23:40, kcc wrote: > On 2011年10月28日 20:02:15, chandlerc wrote: > > It's not immediately obvious to me why this is here, and what problem it > solves? > > Would it be better to just have a global 'const char *' object? > Or just use "llvm.global_ctors"? > Right now, "llvm.global_ctors" is used in 9 different files as a > literal constant I would use "llvm.global_ctors" and maybe add a FIXME comment to put it somewhere appropriate like Support or VMCore. It shouldn't be part of this utilitiy though. http://codereview.appspot.com/5330046/diff/3001/include/llvm/Transforms/Utils... include/llvm/Transforms/Utils/GlobalCtors.h:25: /// Append F to the list of global ctors of module M with the given Priority. On 2011年10月28日 20:23:40, kcc wrote: > On 2011年10月28日 20:02:15, chandlerc wrote: > > This needs better comments. What does 'Priority" mean? what is it actually > used > > for? etc. > > This is documented at http://llvm.org/docs/LangRef.html#intg_global_ctors > Shall I give a link? I would summarize it briefly, and give the link: Append F to the list of global ctors of module M with the given priority. This wraps the function in the appropriate structure and stores it along side other global constructors. See ... for details.