From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25926 invoked by alias); 12 Apr 2010 16:11:41 -0000 Received: (qmail 25908 invoked by uid 22791); 12 Apr 2010 16:11:38 -0000 X-SWARE-Spam-Status: No, hits=-5.5 required=5.0 tests=BAYES_05,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; Mon, 12 Apr 2010 16:11:29 +0000 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o3CGBR6j001154 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 12 Apr 2010 12:11:27 -0400 Received: from fche.csb (vpn-235-68.phx2.redhat.com [10.3.235.68]) by int-mx08.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o3CGBQUG032066; Mon, 12 Apr 2010 12:11:26 -0400 Received: by fche.csb (Postfix, from userid 2569) id 871625814D; Mon, 12 Apr 2010 12:11:25 -0400 (EDT) To: Stan Cox Cc: SystemTap List Subject: Re: debuginfoless static user probes References: <4BC29670.7040809@redhat.com> From: fche@redhat.com (Frank Ch. Eigler) Date: Mon, 12 Apr 2010 16:11:00 -0000 In-Reply-To: <4BC29670.7040809@redhat.com> (Stan Cox's message of "Sun, 11 Apr 2010 23:41:36 -0400") Message-ID: User-Agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2010-q2/txt/msg00091.txt.bz2 Stan Cox writes: > This patch implements adding info to the .probes section to access > probe arguments and since the probe address is also in the .probes > section this means no debuginfo is required for a > process("proc").mark("mark") probe. Thanks. > The .probes section format is now: > %rcx %rsi %rdi > seven__test > UPB1 > pointer to seven__test > count of probe arguments > probe address > pointer to argument asm string You will earn reap manifold benefits if each and every permutation of the .probes struct is available in sys/sdt.h as a full fledged struct declaration. It may be helpful to also include the capability to generate all corresponding STAP_PROBE_DATA() variants, e.g. for use by the testsuite. It would also be good to document clearly the run-time interface ABI at the probe invocation moment, for each of these probes. i.e., kprobe1 style assumes kprobe in sys_geteuid, arg1 being blah, arg2 being blah2, blah blah blah. It's nice to have the arg-count in there finally; this should improve translation-time error handling. > --- a/tapset/i386/registers.stp > +++ b/tapset/i386/registers.stp > @@ -12,12 +12,16 @@ function _stp_register_regs() { > > /* Same order as pt_regs */ > _reg_offsets["ebx"] = 0 _reg_offsets["bx"] = 0 > + _reg_offsets["bl"] = 0 > _reg_offsets["ecx"] = 4 _reg_offsets["cx"] = 4 > + _reg_offsets["cl"] = 4 > [...] Ah an interesting solution: map the register names in the assembly code to strings supplied to the tapset function. This would make these tapset functions more heavily used than usual. Some complications: more testing re. valid and unexpected inputs, treatment of kernel-space vs user-space register sets (see the PR10601 stuff for uprobes). Consider that the expressions / register names would henceforth arrive from untrustworthy user-space binaries! > sdt_var_expanding_visitor(string & process_name, string & probe_name, > - int arg_count, bool have_reg_args): > + string & arg_string, > + int arg_count, bool have_kprobe): (Do those string& args need to be string& as opposed to const string& ? Are those additional outputs?) > [...] > + for (unsigned i = 0; i < tok.length(); i++) > + // Recognize: %R | (%R) | N(%R) > + switch (tok[i]) > + { > + case '-': > + case '1' ... '9': > + { > + disp_str = tok.substr(i,tok.find('(',i)-i); > + i += disp_str.length(); > + disp = lex_cast(disp_str); > + arg_in_memory = true; > + break; > + } > + case '(': > + arg_in_memory = true; > + break; > + case ')': > + break; > + case '%': > + { > + reg = tok.substr(i+1,tok.find(')',i)-i-1); > + i += reg.length(); > + break; > + } > + default: > + bad_asm_op = true; > + } It would seem less fragile to use a mechanism such as regular expressions to parse the thing. (Remember, they are not trustworthy from a security point of view.) > [...] > + literal_number* num = new literal_number(3); > [...] > + literal_number* inc = new literal_number((argno - 1) * 8); There are a couple of similar magic numbers (3, -1, 8) in there that should be better explained. > + if (have_kprobe || arg_in_memory) > + provide(fc); > + else > + provide(be); Just confirming, you are not removing the translator's compatibility with the old style probes, right? It may make the code best if all the alternatives are handled in completely separate code paths in the translator, without intermingling the code with flags like "have_kprobe || arg_in_memory". If there are too many conditionals, the code probably could be improved. This is C++, we can subtype etc. if needed. > @@ -4109,10 +4178,12 @@ sdt_query::handle_query_module() > clog << "matched probe_name " << probe_name << " probe_type "; > switch (probe_type) > { > - case uprobe_type: > - clog << "uprobe at 0x" << hex << probe_arg << dec << endl; > + case uprobe1_type: > + case uprobe2_type: > + clog << "uprobe at 0x" << hex << pc << dec << endl; > break; > - case kprobe_type: > + case kprobe1_type: > + case kprobe2_type: > clog << "kprobe" << endl; > break; > } We lose diagnostic capabilities if uprobe1/uprobe2 are shown indistinguisably here. > + Dwarf_Addr bias; > + Elf* elf = (dwarf_getelf (dwfl_module_getdwarf (q.dw.mod_info->mod, &bias)) > + ?: dwfl_module_getelf (q.dw.mod_info->mod, &bias)); > + > + GElf_Ehdr ehdr_mem; > + GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem); > + string section; > + if ((em->e_type == ET_EXEC)) > + section = ".absolute"; > + else if (em->e_type == ET_DYN) > + section = ".dynamic"; > + uprobe_derived_probe* p = > + new uprobe_derived_probe ("", "", 0, q.module_val, section /*".absolute"*/, > + q.statement_num_val, q.statement_num_val, > + q, 0); > + results.push_back (p); This looks OK, but will need the sdt.exp style permutation testing with various flavours of executables / shared libraries (pie/prelink). > + sess.unwindsym_modules.insert ("kernel"); (Why?) > + if (probe_type == uprobe1_type || probe_type == kprobe1_type) > + { > + struct probe_entry > + { > + __uint64_t name; > + __uint64_t arg; > + } *pbe; > + > + probe_scn_offset += sizeof(__uint32_t); > + probe_scn_offset += probe_scn_offset % sizeof(__uint64_t); > + pbe = (struct probe_entry*) ((char*)pdata->d_buf + probe_scn_offset); > + if (pbe->name == 0) > + return false; > + probe_name = (char*)((char*)pdata->d_buf + pbe->name - (char*)probe_scn_addr); All this pointer goo could go away if sdt.h contained usable declarations for the .probe structs. Thanks, good progress. - FChE