From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17396 invoked by alias); 10 Sep 2010 15:39:32 -0000 Received: (qmail 17387 invoked by uid 22791); 10 Sep 2010 15:39:31 -0000 X-SWARE-Spam-Status: No, hits=-6.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_GJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 10 Sep 2010 15:39:24 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o8AFdMMv006029 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 10 Sep 2010 11:39:22 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o8AFdLAs019562; Fri, 10 Sep 2010 11:39:22 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o8AFdJMG016749; Fri, 10 Sep 2010 11:39:20 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 45CB53792BD; Fri, 10 Sep 2010 09:39:19 -0600 (MDT) From: Tom Tromey To: Serge CHATROUX Cc: "gdb-patches\@sourceware.org" Subject: Re: GDB 7.2 - Patch proposal for the use of GDB/Python scripts on MinGW References: Date: Fri, 10 Sep 2010 15:47:00 -0000 In-Reply-To: (Serge CHATROUX's message of "Fri, 10 Sep 2010 15:25:45 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-09/txt/msg00225.txt.bz2 >>>>> "Serge" == Serge CHATROUX writes: Thank you for doing this. Serge> I compile gdb 7.2 using MINGW tools and I enable the Python Serge> scripting support by giving the path to a Python for Windows Serge> install directory. Do you have a copyright assignment in place for gdb? If not, email me and we can get the process started. Serge> I also modify the gdb/python sources to solve the following issues: It is generally better if you send separate patches for each thing. Also, we like submissions to include a ChangeLog entry... see src/gdb/CONTRIBUTE for various other things. Serge> + #ifdef __MINGW32__ Serge> + /* Serge> + I have to modify some gdb python files because the files 'python-function.c', python-cmd.c and python-frame.c define some 'static PyTypeObject'. Serge> + The field 'tp_new' of these static variables statically initialized with the python 'PyType_GenericNew' function. Serge> + In Windows port of Python, this function is declared as a dllimport function and can not be copied at compilation time in a static variable. Serge> + For these 3 files, I modify the source code to initialize the 'tp_new' fields to '0' in the static variables and to affect the proper 'PyType_GenericNew' value in the 'gdbpy_initialize_commands', 'gdbpy_initialize_frames' and 'gdbpy_initialize_functions'. Serge> + */ Serge> + cmdpy_object_type.tp_new = PyType_GenericNew; Serge> + #endif Just make your patch do this unconditionally on all hosts, with a little (one line -- not as long as what you have above) comment explaining where it is needed. I think that is clearer than using #ifdef all over. This change could be one patch, it could go in very quickly, because I think there is only one way to do this, and so it could bypass the paperwork requirements. Serge> + /* Do not enable python scripting if the PYTHONHOME variable is undefined */ Serge> + int havePython = 1; Should be static. Also GDB doesn't use mixed case like that, so it needs a different name. Serge> + int have_python() Space before the open paren. This change is needed throughout your patch. Also it should read `(void)', not `()'. Serge> *************** eval_python_from_control_command (struct Serge> *** 147,152 **** Serge> --- 154,165 ---- Serge> char *script; Serge> struct cleanup *cleanup; Serge> + /* Do not enable python scripting if the PYTHONHOME variable is undefined */ Serge> + if (havePython == 0) There should not be a way to get here if python is disabled, so I think this could just be an assertion. Serge> *************** python_command (char *arg, int from_tty) [...] Serge> + #ifdef HAVE_PYTHON Serge> + if (havePython == 0) Serge> + { I don't think this #ifdef is needed; actually, this seems to be true of all the tests of HAVE_PYTHON in the patch. There are 2 copies of python_command, and the one you are modifying is already in an #ifdef HAVE_PYTHON Serge> + while (arg && *arg && isspace (*arg)) Serge> + ++arg; Serge> + if (arg && *arg) Serge> + error (_("Python scripting is not supported when the PYTHONHOME variable is undefined.")); Serge> + else Serge> + { Serge> + struct command_line *l = get_command_line (python_control, ""); Serge> + struct cleanup *cleanups = make_cleanup_free_command_lines (&l); Serge> + execute_control_command_untraced (l); Serge> + do_cleanups (cleanups); Serge> + } Serge> + return; Serge> + } Serge> + #endif /* HAVE_PYTHON */ Also I don't think this needs to be duplicated. If !havePython, then both forms of the python command should simply call error. Serge> *************** source_python_script (FILE *stream, cons [...] Serge> + #ifdef HAVE_PYTHON Likewise. Serge> + /* Do not enable python scripting if the PYTHONHOME variable is undefined */ Serge> + if (havePython == 0) Serge> + { Serge> + error (_("Python scripting is not supported when the PYTHONHOME variable is undefined.")); Serge> + return; `error' calls longjmp -- it is an exception facility written in C. So, you don't need this `return' here. Serge> + #ifdef __MINGW32__ Serge> + { Serge> + /* We can not use 'PyRun_SimpleFile' because Python may not be compiled using the same MSVCRT DLL than GDB Serge> + so the FILE* stream will not be known in this DLL. This generate an error when the MSVCRT will try to lock the file handle. */ These lines are too long. In GDB we wrap at column 79 or so; the GNU coding standards cover this. Serge> + PyObject* PyFileObject = PyFile_FromString( (char*) file, "r"); The spacing is different from our standard. It should look like: PyObject *file_object = PyFile_FromString ((char *) file, "r"); Perhaps this code and some surrounding code should be refactored so that we can just avoid FILE* and use the same code path on all hosts. Serge> + /* Do not enable python scripting if the PYTHONHOME variable is undefined */ Serge> + #ifdef HAVE_PYTHON Serge> + if (getenv( "PYTHONHOME") == NULL) Serge> + { This doesn't seem correct to me. It definitely isn't ok for the Linux distro case. There, Python comes with the system and basically nobody sets PYTHONHOME. It isn't clear to me that it is always correct even on MinGW. Couldn't someone ship a pre-built GDB plus a pre-built Python in the same tree, and expect it to work properly? What is the failure mode here? GDB finds the python DLL but still somehow doesn't work? Tom