Usage of getenv() inside LLVM and thread safety

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Usage of getenv() inside LLVM and thread safety

Dirkjan Bussink
Hello,

In Rubinius we're seeing an occasional crash inside LLVM that always happens inside getenv(), which is used for example when creating a MCContext (inside lib/MC/MCContext.cpp, it checks getenv("AS_SECURE_LOG_FILE")).

The problem is that getenv() and friends aren't thread safe and Rubinius provides a multithreaded system. We can relatively easily get locking setup around the getenv() calls we do in Rubinius, but that's really complex to be able to do for LLVM. I also don't know what the guarantees are here that LLVM wants to provide regarding thread safety of code that happens to have a getenv() call inside it.

What would be the best approach to solving this? Would it be necessary to put locking around this inside our usage of LLVM? Should LLVM provide a locking mechanism for this or should there be a way to make the getenv() calls optional in the places there are used (like here in MCContext).

Regarding the thread safety, this is what the open group says about getenv():

"The getenv() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."

http://pubs.opengroup.org/onlinepubs/009696799/functions/getenv.html

--
Regards,

Dirkjan Bussink
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Usage of getenv() inside LLVM and thread safety

Justin Holewinski-2
That sounds like a missed multi-threading issue with LLVM.  I can't imagine why the user should be forced to serialize creation of MCContext objects. I would suggest filing a bug for this.  A simple lock probably wouldn't be too detrimental to performance here, since MCContext objects shouldn't be created too often.


On Thu, May 23, 2013 at 9:49 AM, Dirkjan Bussink <[hidden email]> wrote:
Hello,

In Rubinius we're seeing an occasional crash inside LLVM that always happens inside getenv(), which is used for example when creating a MCContext (inside lib/MC/MCContext.cpp, it checks getenv("AS_SECURE_LOG_FILE")).

The problem is that getenv() and friends aren't thread safe and Rubinius provides a multithreaded system. We can relatively easily get locking setup around the getenv() calls we do in Rubinius, but that's really complex to be able to do for LLVM. I also don't know what the guarantees are here that LLVM wants to provide regarding thread safety of code that happens to have a getenv() call inside it.

What would be the best approach to solving this? Would it be necessary to put locking around this inside our usage of LLVM? Should LLVM provide a locking mechanism for this or should there be a way to make the getenv() calls optional in the places there are used (like here in MCContext).

Regarding the thread safety, this is what the open group says about getenv():

"The getenv() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."

http://pubs.opengroup.org/onlinepubs/009696799/functions/getenv.html

--
Regards,

Dirkjan Bussink
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



--

Thanks,

Justin Holewinski

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Usage of getenv() inside LLVM and thread safety

Reid Kleckner-2
In reply to this post by Dirkjan Bussink
Right.  glibc's amusing stance is that you setenv/putenv are not thread safe, but getenv is.  I assume Ruby exposes setenv and therefore simply not calling setenv isn't an option.

Would it solve your problems if all getenv() calls happened at cl::ParseCommandLineOptions() time?


On Thu, May 23, 2013 at 9:49 AM, Dirkjan Bussink <[hidden email]> wrote:
Hello,

In Rubinius we're seeing an occasional crash inside LLVM that always happens inside getenv(), which is used for example when creating a MCContext (inside lib/MC/MCContext.cpp, it checks getenv("AS_SECURE_LOG_FILE")).

The problem is that getenv() and friends aren't thread safe and Rubinius provides a multithreaded system. We can relatively easily get locking setup around the getenv() calls we do in Rubinius, but that's really complex to be able to do for LLVM. I also don't know what the guarantees are here that LLVM wants to provide regarding thread safety of code that happens to have a getenv() call inside it.

What would be the best approach to solving this? Would it be necessary to put locking around this inside our usage of LLVM? Should LLVM provide a locking mechanism for this or should there be a way to make the getenv() calls optional in the places there are used (like here in MCContext).

Regarding the thread safety, this is what the open group says about getenv():

"The getenv() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."

http://pubs.opengroup.org/onlinepubs/009696799/functions/getenv.html

--
Regards,

Dirkjan Bussink
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Usage of getenv() inside LLVM and thread safety

Sean Silva
In reply to this post by Justin Holewinski-2
On Thu, May 23, 2013 at 8:13 AM, Justin Holewinski <[hidden email]> wrote:
That sounds like a missed multi-threading issue with LLVM.  I can't imagine why the user should be forced to serialize creation of MCContext objects. I would suggest filing a bug for this.  A simple lock probably wouldn't be too detrimental to performance here, since MCContext objects shouldn't be created too often.

The deeper question is why we are even checking a "global" here in the first place? It goes against LLVM's library-based design. So I don't think introducing locking around this is the "right" solution. Can we just make this value an argument to the constructor?

-- Sean Silva

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Usage of getenv() inside LLVM and thread safety

Justin Holewinski-2
This would require threading the value through some LLVMTargetMachine functions, at the least.  Do we still lock and check the env. var. if the given value is NULL?  Or just do away with the default coming from the env. var?


On Thu, May 23, 2013 at 2:30 PM, Sean Silva <[hidden email]> wrote:
On Thu, May 23, 2013 at 8:13 AM, Justin Holewinski <[hidden email]> wrote:
That sounds like a missed multi-threading issue with LLVM.  I can't imagine why the user should be forced to serialize creation of MCContext objects. I would suggest filing a bug for this.  A simple lock probably wouldn't be too detrimental to performance here, since MCContext objects shouldn't be created too often.

The deeper question is why we are even checking a "global" here in the first place? It goes against LLVM's library-based design. So I don't think introducing locking around this is the "right" solution. Can we just make this value an argument to the constructor?

-- Sean Silva



--

Thanks,

Justin Holewinski

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Usage of getenv() inside LLVM and thread safety

Chris Lattner-2
In reply to this post by Sean Silva

On May 23, 2013, at 11:30 AM, Sean Silva <[hidden email]> wrote:

On Thu, May 23, 2013 at 8:13 AM, Justin Holewinski <[hidden email]> wrote:
That sounds like a missed multi-threading issue with LLVM.  I can't imagine why the user should be forced to serialize creation of MCContext objects. I would suggest filing a bug for this.  A simple lock probably wouldn't be too detrimental to performance here, since MCContext objects shouldn't be created too often.

The deeper question is why we are even checking a "global" here in the first place? It goes against LLVM's library-based design. So I don't think introducing locking around this is the "right" solution. Can we just make this value an argument to the constructor?

MCContext should definitely not be calling getenv.  Yes, it should just be an argument to the ctor, or perhaps a "setter" method on mccontext, used by clients who care.

-Chris


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Usage of getenv() inside LLVM and thread safety

Dirkjan Bussink
In reply to this post by Reid Kleckner-2
Yeah, Ruby allows basically for arbitrary getenv / setenv calls, so they can happen at any time. But since is though the Ruby API's, we can lock around this to prevent issues like this. Not really possible with MCContext, unless locking with the same lock around that as well then.

I think ParseCommandLineOptions would be fine, since we don't use that and I guess it's a one time thing to run at startup then right?

--
Dirkjan

On May 23, 2013, at 16:15 , Reid Kleckner <[hidden email]> wrote:

> Right.  glibc's amusing stance is that you setenv/putenv are not thread safe, but getenv is.  I assume Ruby exposes setenv and therefore simply not calling setenv isn't an option.
>
> Would it solve your problems if all getenv() calls happened at cl::ParseCommandLineOptions() time?
>
>
> On Thu, May 23, 2013 at 9:49 AM, Dirkjan Bussink <[hidden email]> wrote:
> Hello,
>
> In Rubinius we're seeing an occasional crash inside LLVM that always happens inside getenv(), which is used for example when creating a MCContext (inside lib/MC/MCContext.cpp, it checks getenv("AS_SECURE_LOG_FILE")).
>
> The problem is that getenv() and friends aren't thread safe and Rubinius provides a multithreaded system. We can relatively easily get locking setup around the getenv() calls we do in Rubinius, but that's really complex to be able to do for LLVM. I also don't know what the guarantees are here that LLVM wants to provide regarding thread safety of code that happens to have a getenv() call inside it.
>
> What would be the best approach to solving this? Would it be necessary to put locking around this inside our usage of LLVM? Should LLVM provide a locking mechanism for this or should there be a way to make the getenv() calls optional in the places there are used (like here in MCContext).
>
> Regarding the thread safety, this is what the open group says about getenv():
>
> "The getenv() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."
>
> http://pubs.opengroup.org/onlinepubs/009696799/functions/getenv.html
>
> --
> Regards,
>
> Dirkjan Bussink
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev