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

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

This looks like an entirely correct change to me. LGTM, please submit after fixing the below comment issue.

Joerg, I'm not really concerned about the issues you raise for a few reasons.

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

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.

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 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.

-Chandler


================
Comment at: lib/Support/Unix/Process.inc:357
@@ +356,3 @@
+  // Save the current term (if set) to be restored after the test.
+  // See PR25007.
+  struct term *OrigTerm = set_curterm((struct term *)nullptr);
----------------
Please don't cite PRs in comments. The comment line above is sufficient.


http://reviews.llvm.org/D15077