add utils to manipulate global ctors

[kcc, 2011-10-28 19:41:01]

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.

[chandlerc, 2011-10-28 20:02:14]

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.

[kcc, 2011-10-28 20:23:40]

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.

[chandlerc, 2011-10-28 20:27:28]

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.