[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 23:43:20 CDT 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