[PATCH] D15077: [PATCH] Save and restore current term setting in terminalHasColors(). PR25007

[Joerg Sonnenberger via llvm-commits]

joerg added a comment.

In http://reviews.llvm.org/D15077#306894, @chandlerc wrote:

> First and foremost, this is already *always* done under a single global lock for the entire
>  address space. So we can't get correctness issues of tearing here from other callers of these routines.


I never said that. I was always talking about other applications using the LLVM library.

> Second, this is file descriptor specific and so it doesn't seem unreasonable to call it

>  multiple times, and *this* level isn't the level which should do the caching. We might

>  want to do that in raw_ostream or elsewhere, but that's really orthogonal to the issue

>  this patch fixes.


Actually, it isn't. setupterm needs a file descriptor, but only to remember it for following
operations and maybe change baud rate. The data we care about with the color codes is completely independent of that.

> The remaining concern is the fact that there could be some other thread which is doing its

>  own concurrent calls to setupterm and  because those won't use the same global variables,

>  they will race with these calls. True, but unavoidable. Callers of the *HasColors routines

>  provided by Process.h need to be aware that they are calling a routine which is not safe

>  to call concurrently with arbitrary other systems code. I don't think this is the layer to protect

>  the callers from such failure modes.


The patch changes the failure mode from "corruption always happens" to "corruption happens depending on the use order". The very need for the patch illustrates that the problem is well hidden and that library users are not generally aware of this dependency. That seems to be a pretty valid concern
about the implications of the patch.

> The correct way for all of this to work is for the Driver library in Clang to accept already set up

>  (and likely cached color state) raw_ostream objects, and for the same chunk of code as has

>  main() in it (tools/driver/...) to set up that ostream for the Driver library. Then the user of the

>  library can decide whether or not to access these routines. But still, this patch seems like a strict

>  improvement. It should go in.


Well, do we have such interfaces or not? Without them, I don't see the patch as a strict improvement...


http://reviews.llvm.org/D15077