[llvm-commits] [llvm] r81826 - in /llvm/trunk: include/llvm/System/Program.h lib/System/Unix/Program.inc lib/System/Win32/Program.inc
Daniel Dunbar
daniel at zuster.org
Mon Sep 21 21:43:20 PDT 2009
Hi Mikhail,
This isn't safe, for exactly the same reasons as I fixed it before here:
http://llvm.org/viewvc/llvm-project?view=rev&revision=77953
and it breaks ExecuteAndWait in exactly the same way. I'm reverting it
for now. See http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20090727/083264.html
If the motivation is supporting GetProcessId pre XP then this should
be recorded in the Program object. I also see no reason to make a
Program object copyable -- what would this mean? The program object
should essentially be opaque, it is implemented by the System layer,
which may well be non-copyable.
I know we don't currently have a good way of running tests on Windows,
but please verify that ExecuteAndWait doesn't break when modifying
this code...
- Daniel
On Mon, Sep 14, 2009 at 8:39 PM, Mikhail Glushenkov <foldr at codedgers.com> wrote:
> Author: foldr
> Date: Mon Sep 14 22:39:45 2009
> New Revision: 81826
>
> URL: http://llvm.org/viewvc/llvm-project?rev=81826&view=rev
> Log:
> Get rid of GetProcessId in Win32/Program.inc.
>
> GetProcessId was introduced only in XP. As a bonus, this change makes Program
> objects copyable, since Program is now basically a PID.
>
> Modified:
> llvm/trunk/include/llvm/System/Program.h
> llvm/trunk/lib/System/Unix/Program.inc
> llvm/trunk/lib/System/Win32/Program.inc
>
> Modified: llvm/trunk/include/llvm/System/Program.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/System/Program.h?rev=81826&r1=81825&r2=81826&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/System/Program.h (original)
> +++ llvm/trunk/include/llvm/System/Program.h Mon Sep 14 22:39:45 2009
> @@ -29,22 +29,18 @@
> /// @since 1.4
> /// @brief An abstraction for finding and executing programs.
> class Program {
> - /// Opaque handle for target specific data.
> - void *Data_;
>
> - // Noncopyable.
> - Program(const Program& other);
> - Program& operator=(const Program& other);
> + unsigned Pid_;
>
> /// @name Methods
> /// @{
> public:
>
> - Program();
> - ~Program();
> + Program() : Pid_(0) {}
> + ~Program() {}
>
> /// Return process ID of this program.
> - unsigned GetPid() const;
> + unsigned GetPid() const { return Pid_; }
>
> /// This function executes the program using the \p arguments provided. The
> /// invoked program will inherit the stdin, stdout, and stderr file
>
> Modified: llvm/trunk/lib/System/Unix/Program.inc
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/System/Unix/Program.inc?rev=81826&r1=81825&r2=81826&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/System/Unix/Program.inc (original)
> +++ llvm/trunk/lib/System/Unix/Program.inc Mon Sep 14 22:39:45 2009
> @@ -34,15 +34,6 @@
> namespace llvm {
> using namespace sys;
>
> -Program::Program() : Data_(0) {}
> -
> -Program::~Program() {}
> -
> -unsigned Program::GetPid() const {
> - uint64_t pid = reinterpret_cast<uint64_t>(Data_);
> - return static_cast<unsigned>(pid);
> -}
> -
> // This function just uses the PATH environment variable to find the program.
> Path
> Program::FindProgramByName(const std::string& progName) {
> @@ -214,7 +205,7 @@
> break;
> }
>
> - Data_ = reinterpret_cast<void*>(child);
> + Pid_ = child;
>
> return true;
> }
> @@ -226,7 +217,7 @@
> #ifdef HAVE_SYS_WAIT_H
> struct sigaction Act, Old;
>
> - if (Data_ == 0) {
> + if (Pid_ == 0) {
> MakeErrMsg(ErrMsg, "Process not started!");
> return -1;
> }
> @@ -242,8 +233,7 @@
>
> // Parent process: Wait for the child process to terminate.
> int status;
> - uint64_t pid = reinterpret_cast<uint64_t>(Data_);
> - pid_t child = static_cast<pid_t>(pid);
> + pid_t child = Pid_;
> while (wait(&status) != child)
> if (secondsToWait && errno == EINTR) {
> // Kill the child.
> @@ -291,15 +281,12 @@
>
> bool
> Program::Kill(std::string* ErrMsg) {
> - if (Data_ == 0) {
> + if (Pid_ == 0) {
> MakeErrMsg(ErrMsg, "Process not started!");
> return true;
> }
>
> - uint64_t pid64 = reinterpret_cast<uint64_t>(Data_);
> - pid_t pid = static_cast<pid_t>(pid64);
> -
> - if (kill(pid, SIGKILL) != 0) {
> + if (kill(Pid_, SIGKILL) != 0) {
> MakeErrMsg(ErrMsg, "The process couldn't be killed!");
> return true;
> }
>
> Modified: llvm/trunk/lib/System/Win32/Program.inc
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/System/Win32/Program.inc?rev=81826&r1=81825&r2=81826&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/System/Win32/Program.inc (original)
> +++ llvm/trunk/lib/System/Win32/Program.inc Mon Sep 14 22:39:45 2009
> @@ -25,21 +25,6 @@
> namespace llvm {
> using namespace sys;
>
> -Program::Program() : Data_(0) {}
> -
> -Program::~Program() {
> - if (Data_) {
> - HANDLE hProcess = reinterpret_cast<HANDLE>(Data_);
> - CloseHandle(hProcess);
> - Data_ = 0;
> - }
> -}
> -
> -unsigned Program::GetPid() const {
> - HANDLE hProcess = reinterpret_cast<HANDLE>(Data_);
> - return GetProcessId(hProcess);
> -}
> -
> // This function just uses the PATH environment variable to find the program.
> Path
> Program::FindProgramByName(const std::string& progName) {
> @@ -137,11 +122,6 @@
> const Path** redirects,
> unsigned memoryLimit,
> std::string* ErrMsg) {
> - if (Data_) {
> - HANDLE hProcess = reinterpret_cast<HANDLE>(Data_);
> - CloseHandle(Data_);
> - Data_ = 0;
> - }
>
> if (!path.canExecute()) {
> if (ErrMsg)
> @@ -269,9 +249,10 @@
> path.str() + "'");
> return false;
> }
> - Data_ = reinterpret_cast<void*>(pi.hProcess);
> + Pid_ = pi.dwProcessId;
>
> // Make sure these get closed no matter what.
> + AutoHandle hProcess(pi.hProcess);
> AutoHandle hThread(pi.hThread);
>
> // Assign the process to a job if a memory limit is defined.
> @@ -305,12 +286,17 @@
> int
> Program::Wait(unsigned secondsToWait,
> std::string* ErrMsg) {
> - if (Data_ == 0) {
> + if (Pid_ == 0) {
> MakeErrMsg(ErrMsg, "Process not started!");
> return -1;
> }
>
> - HANDLE hProcess = reinterpret_cast<HANDLE>(Data_);
> + HANDLE hOpen = OpenProcess(SYNCHRONIZE, FALSE, Pid_);
> + if (hOpen == NULL) {
> + MakeErrMsg(ErrMsg, "OpenProcess failed!");
> + return -1;
> + }
> + AutoHandle hProcess(hOpen);
>
> // Wait for the process to terminate.
> DWORD millisecondsToWait = INFINITE;
> @@ -341,12 +327,18 @@
>
> bool
> Program::Kill(std::string* ErrMsg) {
> - if (Data_ == 0) {
> + if (Pid_ == 0) {
> MakeErrMsg(ErrMsg, "Process not started!");
> return true;
> }
>
> - HANDLE hProcess = reinterpret_cast<HANDLE>(Data_);
> + HANDLE hOpen = OpenProcess(PROCESS_TERMINATE, FALSE, Pid_);
> + if (hOpen == NULL) {
> + MakeErrMsg(ErrMsg, "OpenProcess failed!");
> + return true;
> + }
> + AutoHandle hProcess(hOpen);
> +
> if (TerminateProcess(hProcess, 1) == 0) {
> MakeErrMsg(ErrMsg, "The process couldn't be killed!");
> return true;
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list