From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28881 invoked by alias); 24 Feb 2012 22:56:12 -0000 Received: (qmail 28817 invoked by uid 22791); 24 Feb 2012 22:56:09 -0000 X-SWARE-Spam-Status: No, hits=-6.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,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, 24 Feb 2012 22:55:51 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1OMtlKt026116 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 24 Feb 2012 17:55:47 -0500 Received: from [10.3.113.116] (ovpn-113-116.phx2.redhat.com [10.3.113.116]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q1OMtkVA003041; Fri, 24 Feb 2012 17:55:47 -0500 Message-ID: <4F481572.9080706@redhat.com> Date: Fri, 24 Feb 2012 22:56:00 -0000 From: Josh Stone User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120209 Thunderbird/10.0.1 MIME-Version: 1.0 To: Wade Farnsworth CC: systemtap@sourceware.org Subject: Re: [PATCH v2] PR12331: Introduce stap options --sysroot and --sysenv References: <4F0AFEEE.30909@mentor.com> <4F340FD7.9010205@mentor.com> In-Reply-To: <4F340FD7.9010205@mentor.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2012-q1/txt/msg00197.txt.bz2 Looks pretty good. I found a few things in review, noted below, but otherwise I think we can go ahead and push this into the repo, and patch any other issues as they arise. We should have at least some minimal testing for these options. It's hard since the testsuite can't assume any location of a real sysroot, but at least it should be sanity checked that something like this doesn't break: stap --sysroot=/ --sysenv=PATH=$PATH ... Docs are needed too -- please update stap.1 and NEWS. Thanks, Josh (review is heavily snipped, but hopefully it makes sense...) On 02/09/2012 10:26 AM, Wade Farnsworth wrote: > // Hash compiler path, size, and mtime. We're just going to assume > // we'll be using gcc. XXX: getting kbuild to spit out out would be > // better, especially since this is fooled by ccache. > - h.add_path("Compiler ", find_executable("gcc")); > + h.add_path("Compiler ", find_executable("gcc", "", dummy)); Not your fault, but that comment is getting more urgent. (or rather, this way of hashing gcc is getting more irrelevent.) Tracking the file info of a ccache wrapper is not helpful, and this is also missing the boat if CROSS_COMPILE is used. May have to look at kbuild again, or manually emulate kbuild's logic in choosing a compiler... > @@ -86,6 +87,8 @@ systemtap_session::systemtap_session (): > kernel_release = string (buf.release); > release = kernel_release; > kernel_build_tree = "/lib/modules/" + kernel_release + "/build"; > + sysroot = ""; > + update_release_sysroot = false; These two and sysenv should be addressed in the session copy-ctor too. > + case LONG_OPT_SYSROOT: > + if (client_options) { > + cerr << _F("ERROR: %s invalid with %s", "--sysroot", "--client-options") << endl; > + return 1; > + } else { > + const char *spath = canonicalize_file_name (optarg); > + if (spath == NULL) { > + cerr << _F("ERROR: %s is an invalid directory for --sysroot", optarg) << endl; > + return 1; > + } > + > + sysroot = string(spath); > + if (sysroot[sysroot.size() - 1] != '/') > + sysroot.append("/"); > + > + if (update_release_sysroot) > + kernel_build_tree = sysroot + kernel_build_tree; This method of "update_release_sysroot" feels convoluted to me. For one, if --sysroot occurs multiple times, it will get prepended every time. (If there are other reasons to prohibit multiple sysroot, then please check that.) How about instead, just delay sysroot prepending until after all arg processing? I think in passes_0_4() right at the start of pass 0 is better, just before the comment that says: // Now that no further changes to s.kernel_build_tree can occur [...] > + > + debuginfo_path_insert_sysroot(sysroot); This too should probably wait until options are all settled. > DwflPtr > -setup_dwfl_user(const std::string &name) > +setup_dwfl_user(const std::string &name, systemtap_session &s) > { > if (user_dwfl != NULL > && user_modset.size() == 1 Missing code to make use of the new "s" here? > + if (!sess.sysroot.empty() && ((pos = path.find(sess.sysroot)) != string::npos)) > + path.replace(pos, sess.sysroot.length(), "/"); IMHO, this replacement is common enough to deserve a function. > // Canonicalize the path name. > char *cf = canonicalize_file_name (retpath.c_str()); > if (cf) > { > - retpath = string(cf); > + string scf = string(cf); > + if (sysroot.empty()) > + retpath = scf; > + else { > + int pos = scf.find(sysroot); > + if (pos == 0) > + retpath = scf; > + else { > + cerr << _F("ERROR: file %s not in sysroot %s", cf, sysroot.c_str()) << endl; > + exit(1); We need to do better than an exit(1) here, because there will be temporary files and possibly --remote connections that need to be cleaned up. Probably "throw runtime_error" will suffice instead.