Re: [Ltrace-devel] New ltrace release

[Petr Machata]

2017-02-03 0:27 GMT+01:00 Dima Kogan <d...@secretsauce.net>:
> Petr Machata <pmach...@gmail.com> writes:
>
> I can't devote tons of time to this, but I'm happy to do a release at
> least. I think that will be better than the current state. Alioth
> username: dkogan

I gave you admin privs.  Use them wisely and good luck!

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] New ltrace release

[Petr Machata]

The real reason seems to be lack of resources.  I didn't have time to
touch ltrace for almost two years now.  If somebody else is willing to
step in and finalize a release, and the only thing that they are
missing is Alioth permissions, I'll be happy to grant those.  As
things stand, this project is basically dormant.

Thanks,
Petr

2016-04-09 1:27 GMT+02:00 Dima Kogan :
> Thierry fa...@linux.vnet.ibm.com  writes:
>
>> We all agree that new features like dwarf support should be officially
>> integrated in a release.
>> However today the few tests I have made show that dward test is
>> providing not coherent results on x86_64 so I believe that time is
>> missing to have a more complete testing release ...
>> At least on my side I need a few days to fix ppc64 problems with dwarf
>> test ppc64le being in good shape
>> I would be glad to hear from ARM side...
>
> Hi. Sorry it took so long to reply. Are the DWARF tests the main blocker
> here? There're more features the new code has: backtraces for instance.
>
> I just looked at the dwarf tests, and I see a number of failures that
> have appeared because my libc was updated between when the tests were
> written and now.
>
> One issue was a crash, that is fixed by the attached patch. A number of
> other libc changes cause test failures, but I haven't fixed those yet. I
> will probably simply disable those test chunks.
>
> Are you having trouble on your ppc64 box still? Is it worth it for me to
> try to replicate that in emulation?
>
>
> ___
> Ltrace-devel mailing list
> Ltrace-devel@lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] ltrace: update s390 system call tables

[Petr Machata]

2015-11-06 13:28 GMT+01:00 Heiko Carstens :
> just a simple patch for ltrace to add update and fix the system call tables
> on s390. In addition this also slightly improves the script to generate
> the tables.

Thanks, applied.

There was some trickery with the repository.  It seems Alioth now
hosts GITs on scm.alioth.debian.org//git/ltrace/ltrace.git, whereas it
was scm.alioth.debian.org/git/collab-maint/ltrace.git before.  The old
repo refused my commits, I noticed the change, and pushed to the new
repo.  But that didn't have updates since early last year, which I
didn't notice.  So currently your patch is commited all the way back
among commits from like February 2014.  The newer commits are rebased
on top of that.  I don't think it makes much difference, we didn't
have a release in that time period.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] patch : disable broken printf length

[Petr Machata]

2015-11-11 14:34 GMT+01:00 Mike  Dupont :
> Attached a dirty patch to disable the broken printf max string length
> code.

The patch, cleaned up, would make sense to me.

> Take a look at _doprnt from libiberty for an implementation. I am

We parse the formatting string purely to learn about the vararg types.
I'm not sure libiberty is of much help in that regard.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Infinite stream of breakpoint events

[Petr Machata]

2015-07-25 0:14 GMT+02:00 Andreas Schwab :
> When running the attach-process-dlopen test on ppc64 or ppc64le ltrace
> is receiving an endless stream of breakpoint events:
>
> DEBUG: events.c:336: event: BREAKPOINT: pid=17899, addr=0x3fff88d28c04
>
> The last line is repeated infinitely (and ltrace is not interruptable).
> The addr=0x3fff8ff98c04 is the return address from memset (the previous
> insn at 0x3fff8ffa3338 was blr).
>
> This was uncovered by commit bf82100 (Fix address biasing in PPC
> backend), but the bug is older than that.  Bisecting while
> cherry-picking bf82100 on every candidate identified 73b85aa (Support
> tracing P_PPC64_JMP_IREL slots) as the bad commit.

That code is somewhat tricky, as we use the resolver function as a
breakpoint site.  And memset is an IFUNC I think.  Maybe ltrace sees
the same address for return-from-resolver and return-from-memset.
That could lead to double-setting that breakpoint and lead to the
observed endless loop.  PPC is notorious for tail calls like this.
Unfortunately I don't have access to PPC anymore, so can't really help
out with this.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Fix libunwind support for MIPS

[Petr Machata]

Faraz Shahbazker  writes:

> On 10/22/2015 02:43 AM, Vicente Olivert Riera wrote:
>> ping
>>
>> On 10/08/2015 04:59 PM, Vicente Olivert Riera wrote:
>>> /* Verify that we can safely cast arch_addr_t* to unw_word_t*. */
>>> (void)sizeof(char[1 - 2*(sizeof(unw_word_t) != sizeof(arch_addr_t))]);
>>>
>>> This check will always fail for MIPS 32-bit architectures (the only ones
>>> supported by ltrace) because unw_word_t is 64-bit sized (it's actually a
>>> uint64_t) and arch_add_t (which is void*) is 32-bit sized:
>
> As I understand, that check was deliberately designed to fail at compile time,
> where as your proposal defers it to run time. So one must ask under what
> conditions would the deferral and the related run-time overhead be justified?

I didn't check, but it looks as if on the arches where the compile-time
check would pass in the first place, gcc should be able to figure out
that the assert always passes and not emit any actual code for it.

> If MIPS 32-bit requires a cast-and-compare approach, then we would be
> better off restricting the change to MIPS 32-bit. You have to assume
> that the check is not relevant for mips32 and introduce an
> arch-specific UNW_2_ADDR_T macro which is just a cast in general, but
> calls the conversion+verification function for mips32. Alternatively
> (if the consensus here allows it), you could just assume that the
> check is not valid for mips32 but do it in a way that doesn't affect
> any other architecture.

This would work for me in general, though as I said--is it an actual
problem?  The proliferation of per-arch config tweaks is something
to avoid if possible.

Vincente, would you be able to look at the generated assembly on
e.g. x86_64?  Do one with your proposed patch, and then the same code
with the assert removed, and see if GCC is smart enough to realize the
assert is actually a NOP.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v3] Fix libunwind support for MIPS

[Petr Machata]

Vicente Olivert Riera <vincent.ri...@imgtec.com> writes:

> On 24/07/15 08:38, Petr Machata wrote:
>> Vicente Olivert Riera <vincent.ri...@imgtec.com> writes:
>>
>>>
>>>   rc = unw_get_proc_name(, fn_name,
>>>  sizeof(fn_name),
>>> -   (unw_word_t *) _offset);
>>> +   _function_offset);
>>> +function_offset = (arch_addr_t) uw_function_offset;
>>> +assert(uw_function_offset == (unw_word_t) function_offset);
>>> +
>>>   if (rc == 0 || rc == -UNW_ENOMEM)
>>>   fprintf(options.output, " > %s(%s+%p) [%p]\n",
>>>   lib_name, fn_name, function_offset, ip);
>>
>> This has the same problem.
>
> Then I don't understand what you mean, Petr. function_offset is not
> evaluated in that if instruction...
>
> Could you please be more explicit?

I think I was talking about that assert, but here you initialize
properly, so it should be OK.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v5] Fix libunwind support for MIPS

[Petr Machata]

2015-08-06 12:08 GMT+02:00 Vicente Olivert Riera :
> Tweak the code to make sure the unwind pointers point to the same
> address as the arch ones.

The patch looks good to me overall.

The one outstanding objection that we have is the one regarding the
runtime overhead of the assert.  But I think that compared to the
per-breakpoint overhead of context switch, it's nothing, so unless
Faraz persuades me otherwise, I'll put this in as is.

(But having information about whether GCC actually can optimize this
out would settle this for good.)

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [RFC][PATCH] Add support for mips64 n32/n64

[Petr Machata]

2015-08-04 22:12 GMT+02:00 Faraz Shahbazker <faraz.shahbaz...@imgtec.com>:
> On 07/15/2015 03:24 PM, Petr Machata wrote:
>> It seems like e_machine and e_class should be /replaced/ with some sort
>> of ABI identifier that the backend produces, given an ELF header.  The
>> comment mentions a full-fledged object, but an integer or even a char
>> would very likely be enough.  Having a triplet of machine-class-ABI
>> seems superfluous, the ABI itself subsumes the other two.
>>
>> That's a more invasive change though and if you simply don't have
>> resources to pursue this, I'm not going to push it.  The above is fine.
> While I agree with the above, it seems like something that should be
> part of a general refactoring, so I don't want to muddy the MIPS patch with 
> it.

OK.

>> This should also break if MIPS runs in big endian mode, unless I'm
>> missing something.
> Haven't tested on big-endian, but I think it shouldn't break. For 32-bit 
> process,
> the address stored at DT_MIPS_RLD_MAP is also 32-bit. We should always be 
> able to
> find it within the first 4 relative bytes. OTOH, endianness is not my strong 
> suite,
> so I will double-check.

Please do.

>>> +static int
>>> +abi_select(const int abi, const int list[])
>>> +{
>>> +int retval;
>>> +switch (abi)
>>> +{
>>> +case ABI_N32:
>>> +retval = list[1];
>>
>> Why not use the enumerators as indices?
> No good reason. With C99 designated initializers I'd want to drop abi_select()
> altogether and index the arrays directly in syscall_p(). Does that sound 
> right?

Using C99 is OK.  I think we are around the point where using C11
might actually be worth considering if it happens to make sense.
(Static asserts and _Generic are the two features that seemed
interesting in that standard.)

Referencing the array directly is probably OK as well.

>>> +#ifdef __LP64__
>>> +size_t
>>> +arch_type_sizeof(struct process *proc, struct arg_type_info *info)
>>
>> Define this always, not only on __LP64__.  Make the size contingent on
>> proc->e_machine, proc->e_class or proc->e_abi.
> Defining always is fine, I am not quite clear on the second point.

I think what I had in mind was, don't ifdef on LP64, instead implement
always, and where sizes would differ between 32 and 64 bit, make a
runtime condition.  I.e. have it runtime-selected, drop all the ifdefs
that you possibly can.  Have the compiler see as much code as
possible, so that it can catch bugs from other configurations as well.
(Not that C is that great at static typing, but still.)

> The aberration from normal case (where size is simply sizeof) is
> only necessary for tracing 32-bit process with 64-bit ltrace, so __LP64__
> will have to figure in the check in addition to say proc->e_class.
> I've used mask_32bit flag because it exactly captures both __LP64__ and
> (proc->e_class == ELFCLASS32).

I'm not sure what you are trying to say here.  But if you look at x86
or ppc backend, those have one arch_type_sizeof for both 32-bit and
64-bit ABI(s), having a bunch of constants for everything but long,
and for long they have proc->e_machine == 64-bit one ? 8 : 4.  Does
the condition need to be much more involved than this?

Abolishing conditional compilation is important to me, as this stuff
can easily get out of hand and make a terrible mess of exceptions
within exceptions.

(In my ideal world, the interface to machine-specific parts would be
tiny and be essentially just fetching registers.  90+% of ltrace can
be expressed in platform-independent C.  Having large swaths of logic
hidden in sysdeps/ or conditionally compiled makes large-scale changes
and refactoring in the core difficult and guaranteed to break some
backend.)

Thank you,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] ltrace on ppc64

[Petr Machata]

Andreas Schwab sch...@linux-m68k.org writes:

 Petr Machata pmach...@gmail.com writes:

 It probably should ignore section symbols.  I can't decide on the
 account of object symbols.  I don't know off hand if anything that
 ltrace can actually trace might be represented as STT_OBJECT and we
 should therefore let backends see it.  But it seems that we should
 ignore them also.

 Perhaps arch_translate_address for ppc64 should just ignore everything
 that's not in the range of .opd (instead of returning an error)?

Makes sense.  Such patch would be acceptable.  Unfortunately I don't
have a working PPC anymore, so won't be able to write and test this
myself.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] Fix libunwind support for MIPS

[Petr Machata]

Vicente Olivert Riera vincent.ri...@imgtec.com writes:

 - int rc = unw_get_reg(cursor, UNW_REG_IP,
 -  (unw_word_t *) ip);
 + int rc = unw_get_reg(cursor, UNW_REG_IP, uw_ip);
 + ip = (arch_addr_t) uw_ip;
 + assert(uw_ip == (unw_word_t) ip);
 +
   if (rc  0) {

Sorry, I know this is literally what I said should be done, but when
unw_get_reg fails, uw_ip should be considered uninitialized and the
following assert might fail.  Please move the assignment and the assert
behind the condition block.

Thank you,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Fix libunwind support for MIPS

[Petr Machata]

Vicente Olivert Riera vincent.ri...@imgtec.com writes:

 /* Verify that we can safely cast arch_addr_t* to unw_word_t*. */
 (void)sizeof(char[1 - 2*(sizeof(unw_word_t) != sizeof(arch_addr_t))]);

 This check will always fail for MIPS 32-bit architectures (the only ones
 supported by ltrace) because unw_word_t is 64-bit sized (it's actually a
 uint64_t) and arch_add_t (which is void*) is 32-bit sized:

 output.c:784:3: error: size of unnamed array is negative

 However, since unw_word_t is always 64-bit sized for any MIPS
 architecture, the casting from arch_addr_t to unw_word_t will always be
 possible, so let's disable this check for MIPS architectures.

The problem is casting of one pointer type to another.  The fact that
unw_word_t is 64-bit while arch_addr_t 32-bit is exactly the problem,
because a code seeing unw_word_t* might validly expect that it points to
a 64-bit quantity, while in reality it's only backed by a 32-bit one.

You might build without unwinding support, or alternatively if this
bothers you, tweak the code so that instead of this:

(void)sizeof(char[1 - 2*(sizeof(unw_word_t) != sizeof(arch_addr_t))]);
[...]
arch_addr_t ip;
int rc = unw_get_reg(cursor, UNW_REG_IP, (unw_word_t *) ip);

It does something like this:

uwn_word_t uw_ip;
int rc = unw_get_reg(cursor, UNW_REG_IP, uw_ip);
arch_addr_t ip = (arch_addr_t) ip;
assert(uw_ip == (uwn_word_t) up);

And similarly for unw_get_proc_name.  Such patch would be acceptable as
far as I'm concerned.  (Though that doesn't say much, as I've been
neglecting ltrace lately.)

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Install lib*-types.conf

[Petr Machata]

  dist_pkgdata_DATA = etc/syscalls.conf etc/libc.so.conf   \
 - etc/libm.so.conf etc/libacl.so.conf etc/libpthread.so.conf
 + etc/libm.so.conf etc/libacl.so.conf etc/libpthread.so.conf \
 + etc/libpthread.so-types.conf etc/libc.so-types.conf

Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [RFC][PATCH] Add support for mips64 n32/n64

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 diff --git a/proc.h b/proc.h
 index a611456..00094e1 100644
 --- a/proc.h
 +++ b/proc.h
 @@ -117,6 +117,7 @@ struct process {
* nauseam.  */
   short e_machine;
   char e_class;
 + char e_abi;

So the part that the diff program snipped says this:

/* XXX We would like to replace this with a pointer to ABI
 * object that would provide the relevant services, instead of
 * checking the necessary flags in the back end ad
 * nauseam.  */

It seems like e_machine and e_class should be /replaced/ with some sort
of ABI identifier that the backend produces, given an ELF header.  The
comment mentions a full-fledged object, but an integer or even a char
would very likely be enough.  Having a triplet of machine-class-ABI
seems superfluous, the ABI itself subsumes the other two.

That's a more invasive change though and if you simply don't have
resources to pursue this, I'm not going to push it.  The above is fine.

 --- a/sysdeps/linux-gnu/mips/plt.c
 +++ b/sysdeps/linux-gnu/mips/plt.c
 @@ -182,6 +183,11 @@ arch_find_dl_debug(struct process *proc, arch_addr_t 
 dyn_addr,
  {
   arch_addr_t rld_addr;
   int r;
 +#ifdef __LP64__
 + size_t addrsize = proc-mask_32bit ? 4 : (sizeof *ret);
 +#else /* !__LP64__ */
 + size_t addrsize = sizeof *ret;
 +#endif /* !__LP64__ */
  
   /* MIPS puts the address of the r_debug structure into the
* DT_MIPS_RLD_MAP entry instead of into the DT_DEBUG entry.  */
 @@ -189,7 +195,7 @@ arch_find_dl_debug(struct process *proc, arch_addr_t 
 dyn_addr,
DT_MIPS_RLD_MAP, rld_addr);
   if (r == 0) {
   if (umovebytes(proc, rld_addr,
 -ret, sizeof *ret) != sizeof *ret) {
 +ret, addrsize) != addrsize) {

I wonder if you can rely on the fact that *ret will be initialized to
zeroes.  Otherwise, for 64-bit ltrace tracing a 32-bit process, you
should see garbage in the part that's not overwritten.  You might want
to stash a *ret = 0 in front of the umovebytes.

This should also break if MIPS runs in big endian mode, unless I'm
missing something.

Also, you can meld the line into previous, it would be  80 chars.

 @@ -295,14 +301,25 @@ arch_elf_init(struct ltelf *lte, struct library *lib)
  
   for (j = 0; j  data-d_size / 16; ++j) {
   uint32_t insn;
 + int got_size = 4;
 + uint32_t load_inst = 0x2418U; /* addui t8,0,xx  */
 +
 +#ifdef __LP64__
 + if (arch_get_abi(lte-ehdr) == ABI_N64
 + || arch_get_abi(lte-ehdr) == ABI_O64) {
 + got_size = 8;
 + load_inst = 0x6418U; /* daddui t8,0,xx  */
 + }
 +#endif /* __LP64__ */

Is there a reason that this needs to be between ifdefs?  I'd like to
have as much code exposed to compilation all the time as possible.

 @@ -362,8 +379,17 @@ read_got_entry(struct process *proc, GElf_Addr addr, 
 GElf_Addr *valp)
  {
   /* XXX double cast.  */
   arch_addr_t a = (arch_addr_t) (uintptr_t) addr;
 - uint32_t l;
 - if (proc_read_32(proc, a, l)  0) {
 + uint64_t l = 0;
 + int result;
 +
 +#ifdef __LP64__
 + if (!proc-mask_32bit)
 + result = proc_read_64(proc, a, l);
 + else
 +#endif /* __LP64__ */
 + result = proc_read_32(proc, a, (uint32_t *) l);

Here as well.  The above is essentially if (proc-e_class ==
ELFCLASS64).

 @@ -503,13 +529,27 @@ jump_to_entry_point(struct process *proc, struct 
 breakpoint *bp)
[...]
 +#ifdef __LP64__

And here.

 diff --git a/sysdeps/linux-gnu/mips/trace.c b/sysdeps/linux-gnu/mips/trace.c
 index e81b374..d54818e 100644
 --- a/sysdeps/linux-gnu/mips/trace.c
 +++ b/sysdeps/linux-gnu/mips/trace.c

 +/**
 +   \param abi ABI of current process, from mips_abi_type enum
 +   \param list An array of 4 elements, each corresponding to an ABI, in
 +   the order: o32, n32, n64, o64
 +
 +   return value from array corresponding to requested ABI
 + */
 +static int
 +abi_select(const int abi, const int list[])
 +{
 + int retval;
 + switch (abi)
 + {
 + case ABI_N32:
 + retval = list[1];

Why not use the enumerators as indices?

 + const int rt_sigreturn_list[] = {193, 211, 211, 193};
 + const int sigreturn_list[] = {119, -1, -1, 119};

Use C99 designated initializers:

const int rt_sigreturn_list[] = {
[ABI_O32] = 193, /* etc. */
};

That makes the parameter passing convention a bit less magic.

 + /* get the user's pc (plus 8) */

You mean plus 4?

 +   The above is not necessary in Linux kernel  v2.6.35. Recent
 +   kernels have a fancy-pants method of restarting syscalls.

:)

 +#ifdef __LP64__
 +size_t
 +arch_type_sizeof(struct process *proc, struct arg_type_info *info)

Define this always, not only on __LP64__.  Make the size contingent on
proc-e_machine, proc-e_class or 

Re: [Ltrace-devel] [PATCH 1/2] The return value of process_line is never used, so make it void

[Petr Machata]

Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH 2/2] Add a way to force a line to be interpreted as a function prototype

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 Since import is not a keyword in C, it might be used as a type name.
 However, a function prototype with import as the return type would
 be interpreted as an import directive. So provide a new keyword,
 function, that can be used to force a line to be interpreted as a
 prototype.

 Naturally, the new keyword will also work if the return type is
 function or typedef.

Great stuff.  Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Fix a memory leak in parse_typedef_name

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 --- a/read_config_file.c
 +++ b/read_config_file.c
 @@ -387,6 +387,7 @@ parse_typedef_name(struct protolib *plib, struct locus 
 *loc, char **str)
   return NULL;
  
   struct named_type *nt = protolib_lookup_type(plib, buf, true);
 + free(buf);
   if (nt == NULL)
   return NULL;
   return nt-info;

This got into spam.  Applied now, thanks!

Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Add more libpthread.so definitions

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 Enclosing as an attachment due to potential line wrapping issues.

Pushed.  Thanks,
Petr
___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH 4/6] Split type definitions from the bundled configs into their own files

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 http://lists.alioth.debian.org/pipermail/ltrace-devel/2015-April/001298.html.

Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH 1/6] Fix a memory leak in protolib_cache_maybe_load

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 -   if (DICT_FIND_VAL(cache-protolibs, key, retp) == 0)
 +   if (DICT_FIND_VAL(cache-protolibs, key, retp) == 0) {
 +   if (*retp != NULL  own_key)
 +   free((void *) key);
 return 0;
 +   }

 if (strdup_if(key, key, !own_key)  0) {
 fprintf(stderr, Couldn't cache %s: %s\n,

 This looks like the wrong fix.  The one who allocated the key should
 free it if protolib_cache_maybe_load returns a failure.

 But it returns a success here.

You are right, I didn't notice.  Then it's fine.  I'll push later today
together with the other fixes.

 In fact, I think we should take ownership of the key whenever the
 function returns zero, no matter what's in *retp. What do you think?

Yeah, if a key is passed in with own_key != 0, and the function returns
zero (i.e. a success), the caller should assume the key has been
consumed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH 2/6] Make sure there's at least one space after typedef when parsing

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 For the record, I thought this was okay, because eat_spaces doesn't
 recognize tabs either. You might want to fix that, too.

Good point.  I think that's an omission.  I changed it to use isspace.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH 6/6] Document import directives

[Petr Machata]

Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH 1/6] Fix a memory leak in protolib_cache_maybe_load

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 When a non-NULL protolib is returned and own_key is true, the key must
 be freed.
 ---
  prototype.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/prototype.c b/prototype.c
 index fa52ff3..22e54c4 100644
 --- a/prototype.c
 +++ b/prototype.c
 @@ -547,8 +547,11 @@ protolib_cache_maybe_load(struct protolib_cache *cache,
 const char *key, int own_key, bool allow_private,
 struct protolib **retp)
  {
 - if (DICT_FIND_VAL(cache-protolibs, key, retp) == 0)
 + if (DICT_FIND_VAL(cache-protolibs, key, retp) == 0) {
 + if (*retp != NULL  own_key)
 + free((void *) key);
   return 0;
 + }
  
   if (strdup_if(key, key, !own_key)  0) {
   fprintf(stderr, Couldn't cache %s: %s\n,

This looks like the wrong fix.  The one who allocated the key should
free it if protolib_cache_maybe_load returns a failure.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Fix an uninitialized memory access in parse_lens

[Petr Machata]

I decided name2lens is not really adding much value (it's very simple
and with one call site only), so I inlined the loop to parse_lens.
That's now on master.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Fix XDG_CONFIG_DIRS.exp failing when ltrace needs LD_LIBRARY_PATH to run

[Petr Machata]

Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Fix spurious failures in attach-process.exp

[Petr Machata]

Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] ltrace overhead

[Petr Machata]

yogender nerella ynere...@gmail.com writes:

 I have googled a lot and dont really find any great c++ profiling tools (
 free), do you recommend any?

Check out perf and oprofile.

 ltrace suggests most of our application is spent on memcmp, but by
 looking at our code, we don't see that as a possibility.

I imagine that if there are many short memcmp calls, the overhead would
accumulate to make the function appear dominant.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] ltrace overhead

[Petr Machata]

yogender nerella ynere...@gmail.com writes:

 I am profiling a large application involving oracle database libraries.

 Without any tracing my program completes in 2 to 3 minutes, but with ltrace
 it is taking over 2 to 3 hours.

ltrace is not a great tool for profiling.  Every event implies a context
switch (that's the design of the underlying kernel ptrace interface that
ltrace is using).  That's where the overhead comes from.

 All our code is in the binary, and oracle libraries are dynamically linked.

 What are the command line options, to just profile my application?
 What are the command line options to just profile oracle libraries?

Check out -e.  That's used for selecting which library calls from which
libraries you want displayed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH RESEND] Add POSIX Threads prototypes

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 copious free time.

;)

Thanks,
Petr
___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH RESEND] Add POSIX Threads prototypes

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 Petr Machata pmach...@redhat.com писал в своём письме Thu, 12 Mar
 2015 00:25:36 +0300:

 Also, you don't want to simply call read_config_file recursively, as
 that would re-read the file every time it's included.  You also still
 want to allow the full suite of overrides that ltrace admits,
 i.e. looking through XDG_CONFIG_DIRS et.al.

 I don't think I do, actually! If we do that, then the includer is
 never going to know what it's including. I.e., if a.conf includes
 b.conf, and b.conf is overridden, then the override might not contain
 the stuff that a.conf depends on, so it'll be broken. I think we
 should only search in a.conf's directory in this case.

That's a good point, I didn't think about it this way.

But ultimately my opinion is that it's desirable to allow users to
override this stuff.  As long as they include all the necessary
interfaces, they are fine, and they can correct mistakes, add omissions,
etc.

More importantly, this allows users to reuse the type libraries.  Any
library could do something like import libc.so-types.  That would look
up libc.so-types in /usr/share even if your library is e.g. in your home
directory as you work on it, and you could use things like FILE* etc.

(One particularly useful thing to have would be something like import
next, which would work similarly to RTLD_NEXT in dlopen.  It would
allow users to add bits and pieces to the system config file.  But
that's tangential to the orginial point.)

 The right way about it is to go through protolib cache, which handles
 all this stuff.  So read_config_file would need to get an argument of
 the type struct protolib_cache *.  An include line would trigger a call
 to protolib_cache_load.  The result would be added to the currently
 loaded protolib by protolib_add_import.

 Hmm... I don't really want to include complete protolibs into other
 protolibs, though. Okay, for libc.so.conf and libpthread.so.conf I do,
 but that's an exception, not the rule. For the general case, I
 envision a C-like structure, where there are header files with
 common type definitions, but which aren't necessarily complete
 protolibs themselves. Like this:

 types.inc:
 typedef ldouble = double;

This is a complete protolib that contains one typedef.  That's fine.

 Now, I suppose, we could require the header files to be valid config
 files in themselves (types.inc is), and then we could store them as
 protolibs in the cache, but honestly, I don't see much point in
 that. They're going to be parsed at most once per library, which I
 don't think is a big deal, and if we do cache them, they'll just stay
 in the cache after all the library configs have been parsed, serving
 no purpose.

They could be parsed once per library load, or they could be parsed once
and then looked up.  Protolib cache is simply a work-saving mechanism.
Since this stuff is immutable, there's no reason to parse it repeatedly.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH RESEND] Add POSIX Threads prototypes

[Petr Machata]

Роман Донченко d...@corrigendum.ru writes:

 Hmm... maybe. Could you point me to what you mean by the existing
 low-level functionality? I'm thinking I could add a new type of line
 (e.g. include) and recursively call read_config_file when handling
 one. Is there anything that's already done for me?

What I had in mind is this:

int protolib_add_import(struct protolib *plib, struct protolib *import);

Also, you don't want to simply call read_config_file recursively, as
that would re-read the file every time it's included.  You also still
want to allow the full suite of overrides that ltrace admits,
i.e. looking through XDG_CONFIG_DIRS et.al.

The right way about it is to go through protolib cache, which handles
all this stuff.  So read_config_file would need to get an argument of
the type struct protolib_cache *.  An include line would trigger a call
to protolib_cache_load.  The result would be added to the currently
loaded protolib by protolib_add_import.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Remove void argument lists from libpthread.so.conf

[Petr Machata]

Pushed.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] [MIPS] Detect return from rt_sigreturn syscalls

[Petr Machata]

Sedat Dilek sedat.di...@gmail.com writes:

 Is the PLT-less MIPS binaries support upstream?

It is upstream.  It hasn't been merged do master.

Faraz, just to clear things up, the branch is mostly yours, despite the
name.  I don't know much about MIPS and don't have a Linux-capable
hardware handy.  If you deem the branch ready for master, I can merge
it.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] [MIPS] Detect return from rt_sigreturn syscalls

[Petr Machata]

Pushed.

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] [MIPS] Detect return from rt_sigreturn syscalls

[Petr Machata]

The patch looks good to me.  I'd wait a bit for Eugene feedback so that
you two can figure out any low-hanging fruit, otherwise I'll push
tomorrow.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v3] Tracing PLT-less MIPS binaries

[Petr Machata]

Eugene Rudoy g...@freetz.org writes:

 Is it something already implemented in master? Would merging
 pmachata/mips to master solve the issue? Or should mips backend
 (properly) implement some new-style fetch backend-callbacks?

MIPS would have to implement fetch backend.  I suspect that most of
failures in parameters*.exp are due to this.  You could try writing it
yourself!  It's not hard, and can be done in parallel with other MIPS
work, so you and Faraz wouldn't block each other.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v3] Tracing PLT-less MIPS binaries

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 On 02/09/2015 03:24 AM, Eugene Rudoy wrote:
 I had no problems applying it on top of cf55336f3d. I however had to
 remove an extra ) before  in the following fragment (part of
 arch_dynlink_done) in order to get it compiled.

 That was the only error it seems. I've regenerated the patch anyway.

Applied.

What's the state of MIPS with these patches in?  Do you plan to work on
it some more?  I just checked that we can merge to master trivially, but
I don't know what state that would leave the MIPS backend in.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v3] Tracing PLT-less MIPS binaries

[Petr Machata]

Eugene Rudoy g...@freetz.org writes:

 # result: terminates with snprintf(./ltrace: value.c: 343:
 value_set_word: Assertion `sz = sizeof(value-u.value)' failed.

This looks related to the missing new-style fetch backend.  There's a
small wrapper layer that translates the new-style fetch_* calls to
old-style gimme_arg calls if the backend doesn't provide its own fetch_*
suite.  But gimme_arg is very narrow interface, only allowing passing
through a single word of data.  Hence this assert.

 # test command: ./ltrace /usr/bin/openssl speed 21 | tee openssl_speed.log
 # result: a lot of unexpected breakpoint at-lines
 #   output doesn't contain any call to libssl-functions

This could be anything.  Somehow the internal logic is thrown off and
sees events that it can't make sense of.  After it rejects all other
cases, it assumes it's a breakpoint, but doesn't see one with a given
address in its breakpoint tables.  It could be a misinterpreted
singlestep, unbiased or double-biased calls, or something else still.
Faraz will have to figure this out.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v3] Tracing PLT-less MIPS binaries

[Petr Machata]

Sedat Dilek sedat.di...@gmail.com writes:
 Against which Git branch is this patch?
 pmachata/mips?

That would be the natural choice at this point.

But actually the patch doesn't apply for me as it is.  For some reason
the following hunk is ill-formed:

@@ -406,37 +406,11 @@ arch_elf_add_plt_entry(struct process *proc, struct ltelf 
*lte,

Curiously, I see the following lines in there:

-   libsym-arch.got_entry_addr = got_entry_addr;
[...]
+   libsym-arch.got_entry_addr = got_entry_addr;

That's not something that diff (or git diff) would produce, I reckon.
Did you craft or edit the patch by hand by any chance?  Could you apply
the changes and rediff please?

Looks good otherwise.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] Tracing PLT-less MIPS binaries

[Petr Machata]

Sedat Dilek sedat.di...@gmail.com writes:

 nice to see this making progress.
 Is this still against pmachata/mips Git branch?

Yes.

 Does it need some extra patches you posted?

No.

 Is this RFC?

All patches are ;)

 Can you send out a patch with git send-email even it is RFC

That's what he did.  You can git am the patch on top of pmachata/mips.

 and place (above) some comments?

What parts do you feel are lacking comments?

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] Tracing PLT-less MIPS binaries

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 To capture the last one, I've added a check for whether proc-pid occurs 
 in options.h:opt_p - not sure if there is a simpler way of performing
 this check.

Both a -p and a command can be given.  Probably not something that would
typically be used, but nonetheless this is not a correct way to check
for this.

arch_dylink_done is called either for a process whose execution ltrace
has under control (i.e. initial process or forks of already traced
processes), or always for a process that we attach to.  In theory it can
be called even though arch_dylink_ was not, in fact, _done, there's no
deep logic in ltrace to figure out whether the dynamic linker actually
did finish doing its thing.

Which is a shame, but still you could use the callback to make a mark
somewhere and later check it.  Or create breakpoins as delayed and only
activate them in the callback--that's what PPC does.

 2. mips_unresolved_data is shallow copied for each cloned symbol. If a
 child detaches whilst a symbol remains in NEED_UNRESOLVE state, then
 we shouldn't free its unresolve_data because the parent/siblings may 
 still need it. I've added a ref_count field to the struct to fix this.
 I am not sure how(if at all) PPC tackles this problem.

Looking in, I don't think it does.  That's probably a bug, I'll need to
address that.  Good catch!

The refcounting is fine by me, but for ppc I'll just do deep copy.
Easier to get right, I feel.

 + /* Scan LL-SC range for branches going outside that range */
 + uint32_t spc;
 + for (spc = lladdr + 4; spc  scaddr; spc += 4) {
 + uint32_t scanpcs[2];
 + int snr = mips_next_pcs(proc, spc, scanpcs);
 +
 + if (!inrange(scanpcs[0], lladdr, scaddr)) {
 + if ((newpcs = realloc(newpcs,
 +   (nr + 1) * sizeof(spc))) != NULL)
 + newpcs[nr++] = scanpcs[0];
 + }
 +
 + if ((snr == 2)  !inrange(scanpcs[1], lladdr, scaddr)) {
 + if ((newpcs = realloc(newpcs,
 +   (nr + 1) * sizeof(spc))) != NULL)
 + newpcs[nr++] = scanpcs[1];
 + }
 + }

Sorry for picking yet more nits, but how about replacing the repetition
in the loop with this?

int i;  // actually, ideally unsigned, but you keep it in int
for (i = 0; i  snr; ++i)
if (!inrange(scanpcs[i], lladdr, scaddr))
if ((newpcs = realloc(...)) != NULL)
newpcs[nr++] = scanpcs[i];

Also note that the realloc check is wrong.  If the reallocation fails,
you leak, and newpcs becomes NULL.  In the next branch, the NULL gets
passed to realloc, which I believe is valid and behaves as if you called
malloc.  Thus an error gets muffled and you lose breakpoints.  So really
this should be rather something like:

for (i = 0; i  snr; ++i)
if (!inrange(scanpcs[i], lladdr, scaddr)) {
uint32_t *tmp = realloc(newpcs,
(nr + 1) * sizeof(spc));
if (tmp == NULL)
return -1;
/* And have the caller handle -1.  */

newpcs = tmp;
newpcs[nr++] = scanpcs[i];
}

And looking into it yet more, that sizeof(spc) seems wrong as well.  It
should be sizeof(*newpcs).  The fact that they are both uint32_t is of
course no coincidence, but the sizeof should have a clear indication of
the buffer under allocation.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] Tracing PLT-less MIPS binaries

[Petr Machata]

Sedat Dilek sedat.di...@gmail.com writes:

 Why should I git-apply the patch, extract snippets into this email and
 then comment myself on them - isn't that a bit complicated :-)?

Oh, I see, you would like it inline instead of as an attachment.  Sorry,
I didn't understand what you meant the first time around.  When I expand
the patch, my mailer quotes it in the answer, so I can comment on it as
usual.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Fix missing includes and return statements in test sources

[Petr Machata]

Applied.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] ppc64le filt test hangs

[Petr Machata]

Thierry fa...@linux.vnet.ibm.com thie...@linux.vnet.ibm.com writes:

 Any thought on how to debug what's going on ?

Get a minimal reproducer (ideally tracing a single symbol in a small
binary), then look through -D77, and annotate, annotate, annotate.  You
can in theory debug ltrace under gdb, but I just put there a bunch of
printfs and try to figure out why it's hanging.

If it's a live hang (it's spinning), the problem might be that a
breakpoint is double-set, so that the original instruction is a
brakpoint as well.  Then when a disable request is made, we rewrite one
breakpoint instruction with another, let it run, which of course leads
to immediate stop.  We notice it's on one of our addresses, disable
the breakpoint (which still doesn't do anything), and let it continue.
Et cetera.

If it's a dead hang (it's waiting), it might be stuck in some wait
somewhere.  Typically it's a job control bug.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] Tracing PLT-less MIPS binaries

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 +#define inrange(x,lo,hi) ((x)=(hi)  (x)=(lo))
 +static int
 +mips_atomic_next_pcs(struct process *proc, uint32_t pc, uint32_t *newpcs)
 +{
 + uint32_t spc, lladdr = pc, scaddr = pc + 4;
 + int nr = 0;
 +
 + for (lladdr = pc; scaddr - lladdr = 2048; scaddr += 4) {

The lladdr = pc is not necessary.  Come to think of it, you don't
actually need all those variables.  How about this?

static int
mips_atomic_next_pcs(struct process *proc, uint32_t const lladdr,
 uint32_t *newpcs)
{
int nr = 0;

uint32_t scaddr;
for (scaddr = pc + 4; scaddr - lladdr = 2048; scaddr += 4) {
/* ... */
}

/* ... */

uint32_t spc;
for (spc = lladdr + 4; spc  scaddr; spc += 4) {
/* ... */
/* s/sizeof(pc)/sizeof(lladdr)/g */
}

This way you clearly declare the intent that the variables are loop
iterators.  Formally this is a C99 feature, but GCC has allowed that as
an extension since forever.  Unlike placing the variable declaration in
the loop itself, which would of course be the best way to structure
this:

for (uint32_t scaddr = pc + 4; scaddr - lladdr = 2048; scaddr += 4) {

Alternatively, since none of the iterators is shared, you could have a
single uint32_t addr on top, and reuse that in each of the blocks.

In any case, that's two fewer variables, which is two fewer potentially
moving parts to keep an eye on.

 + newpcs = malloc(2 * sizeof(uint32_t));

In mips_atomic_next_pcs, you use sizeof(pc), which I strongly prefer.
This should be:

newpcs = malloc(2 * sizeof *newpcs);

Looks good otherwise.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Tracing PLT-less MIPS binaries

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 As far as the linker code for MIPS is concerned I don't even see a
 single jump. Any atomic sequence that has a branch can be written as 2
 shorter sequences with the branch decision performed earlier. Besides,
 we always want atomics to be as short as possible. So no, I don't
 expect this to come up in practice.

That's what I would expect.  I suppose whoever implemented this for GDB
just found some code with a jump in there, but really I have no idea
where this comes from.

 Perhaps, I wasn't clear earlier. My intention is just to remove the
 check for (nr = 2) from mips_next_pc(), not to allow more than 2
 breakpoints as a general case. Since the existence of more than 2
 breakpoints is eventually checked by sw_singlestep_add_bp(), enforcing
 the limitation in mips_next_pc is not strictly necessary. So the
 options are:
 1. keep the atomic sequence logic in arch_sw_singlestep(), as it is currently 
 for PPC
 2. move atomic logic to mips_next_pc() and remove the restriction on (nr = 
 2) from mips_next_pc()

Oh, I see now.  I think the PPC way is a bit better, in that it doesn't
encode the upper limit and relies purely on lower-level callbacks.  But
ultimately do whatever makes sense to you.  Just don't overflow newpcs
array if you decide to go the mips_next_pc way.

This check is already problematic:

if (nr = 0 || nr  2)
goto fail;

If nr really is  2, then we are in the domain of undefined behavior
anyway, and the compiler is free to optimize this away.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Tracing PLT-less MIPS binaries

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 On 01/21/2015 02:44 PM, Petr Machata wrote:
 So it seems to me this logic should be folded into mips_next_pcs, and
 only activated when the stepped-over instruction is an LL.
 ... 
 I think ltrace currently support two breakpoints for software
 singlestep.  So you put one just after the SC, and one extra is
 available for when there's a jump between LL and SC.  If there are more
 jumps, we just don't have a good answer for that.  PowerPC just gives up
 and returns SWS_FAIL, which should at least clean up things and get
 ltrace into the right states. 

 PPC sets breakpoints at branch destination for all branches within the
 atomic sequence and relies on the call-back to handle more than 1
 jumps as an error. If we fold the atomic logic in to mips_next_pc,
 we'd have to remove checks on (nr = 2) from mips_next_pc and allow it
 to return more than 2 addresses. Does this sound reasonable?

IIRC, the limit of 2 is taken from GDB, where the PPC
software-singlestepping code is lifted from.  I think it's arbitrary,
very probably it's just empirical upper bound of code found in practice.
Clearly in theory, there can be more than one jump between LL and SC,
but does it come up in practice at all?

If yes, not only the MIPS backend would have to be changed, but the core
as well.  The sofware-singlestepping brakpoints would have to be kept in
a vector probably, instead of a simple array.  It's more book-keeping
and more code.

So if you tell me that's necessary, I can probably find a bit of time to
adapt the core--or you can do it and I'll review your patch.  We'd then
want to be careful to add test cases for it.  It's a messy part of
ltrace, and this use case comes up rarely enough that it might get
broken without anyone noticing.

 makes me think, what if I have a code like this (pardon pseudo-assembly,
 I don't actually speak MIPS ;) )
 
  JMP xyz
  LL
  ... etc ...
 xyz:
  SC

 I can't find the relevant spec for whether this is valid, but gcc does
 not allow LL in a delay slot; it will always insert padding after the

OK, then disregard this.

 jump. Anyway, if this was allowed, there would be no way to handle it
 while single stepping. PC never actually points to the instruction in
 the delay slot so there would be no way to determine the beginning of
 the atomic sequence except by explicit inspecting PC+4, while
 inspecting the branch instruction.

My idea was to inspect PC-4 when stepping over LL, but I admit that I
don't know if it would work.

Anyway, that's now moot.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Tracing PLT-less MIPS binaries

[Petr Machata]

Thanks for the patch.  Most of it looks good, but I have some points
about the software singlestepping.

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 diff --git a/sysdeps/linux-gnu/mips/trace.c b/sysdeps/linux-gnu/mips/trace.c
 index 88e13ac..1e462d0 100644
 --- a/sysdeps/linux-gnu/mips/trace.c
 +++ b/sysdeps/linux-gnu/mips/trace.c
 @@ -277,6 +277,39 @@ arch_sw_singlestep(struct process *proc, struct 
 breakpoint *bp,
  
   while (nr--  0) {
   arch_addr_t baddr = (arch_addr_t) newpcs[nr];
 + arch_addr_t lladdr = baddr;
 + uint32_t inst;
 +
 + /* An atomic Read-Modify-Write, starting with LL and ending with
 +  * SC needs to be treated as a single instruction and stepped
 +  * over, otherwise ERET issued within the SYSCALL will cause the
 +  * write to fail, even for a single thread of execution. LL and
 +  * SC must exist within a 2048-byte contiguous region.
 +  *
 +  * Note: this check is sloppy, in that in only scans forward
 +  * linearly, instead of exploring all possible paths using
 +  * mips_next_pcs; this is deemed sufficient for now, as it
 +  * captures how atomic RMWs are typically coded. */
 + proc_read_32(proc, lladdr, inst);
 + /* Found LL(linked-load), scan ahead for SC(store-conditional) 
 */
 + if (itype_op(inst) == 0x30) {
 + while (lladdr+=4)  {

This is actually

for (; laddr - baddr = 2048; lladdr += 4) {
...

And you can drop the final if down...

 + proc_read_32(proc, lladdr, inst);
 + /* Found SC, now stepover trailing branch */
 + if (itype_op(inst) == 0x38) {
 + uint32_t postrmw[2];
 + if (mips_next_pcs(proc, 
 (uint32_t)lladdr+4,
 +   postrmw)  0)
 + baddr = (arch_addr_t)postrmw[0];
 + break;
 + }
 +
 + /* No SC within 2048b, assume LL is standalone 
 */
 + if (lladdr - baddr  2048)
 + break;

... here.

You also have a couple overlong lines there, please keep the line length
at 80 characters at most.

And one more nit: 2048b reads 2048 bits.  The symbol of a byte is B.

 + }

Correct me if I'm wrong, but this code si there so that you avoid
setting software-singlestepping breakpoints on a LL.  Instead, you go
ahead and put it after the whole LL-SC block.  I don't think that's
necessary.  Only when stepping over LL do you need to do this, so that
you avoid interrupts between LL and SC.

So it seems to me this logic should be folded into mips_next_pcs, and
only activated when the stepped-over instruction is an LL.

You should also check for branches along the way.  If you don't put
breakpoints to branches that escape the LL-SC block, ltrace will lose
control of the process, and may consider the next legitimate breakpoint
a singlestep end, or do something silly like that.  I don't even know
what would happen, but it's likely it would desync the whole tracing
process.

I think ltrace currently support two breakpoints for software
singlestep.  So you put one just after the SC, and one extra is
available for when there's a jump between LL and SC.  If there are more
jumps, we just don't have a good answer for that.  PowerPC just gives up
and returns SWS_FAIL, which should at least clean up things and get
ltrace into the right states.  (But we don't have tests for this, so
it's likely broken one way or another.)

So, I would like your code to handle at least jumps as well.  Hm, which
makes me think, what if I have a code like this (pardon pseudo-assembly,
I don't actually speak MIPS ;) )

JMP xyz
LL
... etc ...
xyz:
SC

Would the delay slot kick in and it would all work as expected?  If yes,
then the jump that you are looking for may be just before the
instruction that you are supposed to single-step over.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Current status of git tree on ppc64le

[Petr Machata]

Thierry fa...@linux.vnet.ibm.com thie...@linux.vnet.ibm.com writes:

  * With GGC 4.9.2 , we get a message:

 /tmp/lt-V2vnjpabM5.c:73:10: note: the ABI of passing
homogeneous float aggregates will change in a future GCC release

Per this commit:

https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01173.html

... it seems like the new convention puts arguments to FPR first, GPR
next, and memory last, while the old (buggy) ABI only put to FPR what it
could and to memory the rest.

But allocate_hfa in sysdeps/linux-gnu/ppc/fetch.c handles this case,
there's an explicit code to use part of r10 for the first argument that
doesn't fit to an FPR.

How comes we are not seeing failures then?  Clearly we have functions in
the test suite that will be impacted by the change.

  * So I am proposing the following patch to get rid of it:

The patch didn't apply using git am, so I commited it under my name
and credited you.  It's not pushed yet, I got distracted by some bugs in
Dwarf backend that that I noticed when re-testing.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Current status of git tree on ppc64le

[Petr Machata]

Petr Machata pmach...@redhat.com writes:

 It's not pushed yet, I got distracted by some bugs in Dwarf backend
 that that I noticed when re-testing.

Pushed now.  The dwarf tests pass for me now on x86_64 Fedora 21.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] dict entries lost due to erase

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 I have observed random failures with PIC, when looking up
 breakpoints. It looks like find_slot() doesn't search far enough in to
 the dictionary before giving up, after an item has been erased.

 Patch enclosed with a simple test-case.

Thanks.  I think most of ltrace still assumes that erase isn't actually
available, so this never came up.

I decided to go with a different fix, because the continue would skip
over the check for loops that's next.  I took your test case
though--thanks for that.  I applied the test case under your name even
though I touched your patch, so speak up if you are uncomfortable what I
ended up committing.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] missing library traces

[Petr Machata]

Faraz Shahbazker faraz.shahbaz...@imgtec.com writes:

 On Fri, Jan 2, 2015 at 1:54 PM, Petr Machata pmachata at redhat.com wrote:
 It seems like I wanted to redo what PowerPC (and newly Xtensa!) was/is
 doing with unresolving PLT slots when they are resolved, so that we
 don't have to move breakpoints to function entry points, but can keep
 them in PLT.  I'm not sure how far I got.  The code for this
 specifically seems to be in, but it is likely that I broke some other
 MIPS use cases, special symbol types or other MIPS magic.

 Someone would need to fix regressions in test cases between
 pmachata/mips and 94773bf0b1 (the branch-off point) and fix them.  They
 would need to have enough domain knowledge to realize where the test
 suite has blind spots, what it doesn't test, and add tests and fixes for
 these test cases.  That someone is unlikely to be me, my personal time
 budget is actually even more strained now than it was a year ago.

 Just started looking at your branch before the holidays. The
 'unresolve' logic for pre-linked ELFs is missing

Yeah, now that I'm looking it, you're right, unprelink isn't written.
The necessary data is stored to libsym-arch.data-got_entry_value and
libsym-arch.data-stub_addr.  The former is what's in the GOT entry now
(i.e. the resolved value, the address of the function entry point), the
latter is what should be there (or rather, what ltrace wants there to be
so that tracing works).  Calling unresolve_got_entry and
mark_as_resolved in mips_stub_bp_install may be all that's needed.

You will probably also need to fill in mips_stub_bp_retract, so that
when ltrace detaches, you patch things back up the way they were.  At
least on PowerPC this was necessary--prelinking modified the binary such
that the normal dynamic resolution path didn't work, and when a call was
made through an unresolved slot, the process would crash.  So you need
to un-unresolve everything again.

 and some of the calculations need to be biased appropriately. I don't

Hmm, that's quite possible.  This is a perennial source of problems.
Really we should type biased and unbiased addresses differently, but C
is not a good language for these sorts of abstractions.

 have the necessary domain knowledge, but I do have time, so if someone
 could nudge me in the right direction ...

I don't really know much about MIPS either.  Mostly what helped me
figure this stuff out was staring at objdump -d, eu-readelf and memory
dumps in gdb.  Which is to say that I'm not sure how much help I'll be.
But do ask, we should be able to figure this out.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [Linux-Xtensa] [PATCH v2] xtensa: add xtensa support

[Petr Machata]

Marc Gauthier m...@cadence.com writes:

 I think overlays map multiple sections at the same address, presumably
 the assumption is that overlays are not used in the context of ltrace?
 (They're not particularly useful with Linux, although one might imagine
 some use in uC-Linux, which we haven't much explored.)

Honestly, I haven't seen overlays used in practice, so this didn't cross
my mind.  Certainly in ltrace, we pretty much assume that addresses
uniquely identify objects.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] missing library traces

[Petr Machata]

Sedat Dilek sedat.di...@gmail.com writes:

 Can you give Oliver appropriates credits in CREDITS file?

Absolutely.  Oliver, send me a patch, please.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v3] xtensa: add xtensa support

[Petr Machata]

Applied.

Thank you,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] xtensa: add xtensa support

[Petr Machata]

Max Filippov jcmvb...@gmail.com writes:

 On Sat, Jan 3, 2015 at 5:53 PM, Petr Machata pmach...@redhat.com wrote:
 Max Filippov jcmvb...@gmail.com writes:
 On Fri, Jan 2, 2015 at 4:32 AM, Petr Machata pmach...@redhat.com wrote:
 +int
 +arch_elf_init(struct ltelf *lte, struct library *lib)
 +{
 + Elf_Scn *scn;
 + GElf_Shdr shdr;
 + GElf_Addr relplt_addr;
 + GElf_Xword relplt_size;
 + GElf_Phdr phdr;
 + size_t i;
 +
 + for (i = 0; gelf_getphdr(lte-elf, i, phdr) != NULL; ++i) {
 + if (phdr.p_type == PT_LOAD) {
 + lib-arch.loadable_sz =
 + phdr.p_vaddr + phdr.p_memsz - lte-bias;

 [...]

 Also, correct me if I'm wrong, but loadable_sz actually points to the
 end of the region, and hence is not a size.  Would loadable_end be a
 better name?

 The intention was to calculate the size of loadable part of a library, to be
 able to tell later, given library base address and a pointer, if it
 points inside
 the library or outside of it.

 Bias doesn't influence size, only the, um, coordinate: where it's
 mapped.

 But anyway: your formula is p_vaddr + p_memsz ± bias.  The first is a
 coordinate, the second is size, adding the two together gives you
 another coordinate (of the end of the buffer pointed to by p_vaddr).
 Bias is again size, so the result is an address, not a size.  Hence why
 I think it's a misnomer.

 Looks like I should have taken the minimal of phdr.p_vaddr and subtract
 it from the maximal phdr.p_vaddr + phdr.p_memsz. It works with bias
 instead of phdr.p_vaddr just because all my libs are not prelinked,
 and bias is in fact a library load address.

Right, highest PT_LOAD's p_vaddr + its p_memsz - lowest PT_LOAD's
p_vaddr.

However, there's this piece in your code:

+   /* Some references may be resolved at this point, they
+* will point outside the loadable area of their own
+* library.  */
+   if (libsym-arch.resolved_addr = base 
+   libsym-arch.resolved_addr - base  sz) {
+   libsym-arch.type = XTENSA_PLT_UNRESOLVED;
+   proc_activate_delayed_symbol(proc, libsym);
+   } else {
+   libsym-arch.type = XTENSA_PLT_RESOLVED;
+   }

Now imagine a DSO, e.g. a very large DSO might exhibit this behavior,
which is biased by less than its size.  I.e. there's an overlap of
addresses, and if a symbol happens to fall into the overlap, your code
would assume it's unresolved even if in fact it already could have been
resolved.

I don't know how much of a problem this would be and don't know in any
case how to fix it, and anyway, it might be theoretical (I don't know
how xtensa maps heap, stack, main binary, DSO's etc., this might not
even come up in practice).  Just something I wanted to draw your
attention to.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] missing library traces

[Petr Machata]

Sedat Dilek sedat.di...@gmail.com writes:

 On Thu, Jan 16, 2014 at 9:20 PM, Petr Machata pmach...@redhat.com wrote:
 Eugene Rudoy g...@freetz.org writes:

 as to compiling the master: I had to apply the two patches attached to this
 mail in order to compile it (I simply removed that
 lte-relplt_count-line).

 Hmm, the _GNU_SOURCE ones probably make sense (see the comment about
 uclibc above one of them).

I pushed the _GNU_SOURCE patch to master now.

 As to fixing the issue, in particular It shouldn't be hard for MIPS'
 arch_elf_init to add any special entries: sounds interesting ;-) but
 it's something completely new to us (Oliver  me), we just cross-compile
 ltrace and make it available to the users of freetz-project.

 Would that be possible for you to take a deeper look into the issue? Is
 there something we can do to support you? Testing, hardware (probably
 remote access only)?

 In fact I'm installing Debian Wheezy inside Qemu as I write this.
 I can't promise anything (this would have to go from my personal time
 budget, which is already fairly strained), but I should be able to at
 least take a look.


 Recently, I was asked some questions about ltrace-0.7.3 MIPS 
 cross-compilation.

 What happened with this issue?
 Is ltrace-0.7.y still BROKEN on MIPS?

 I have seen a pmachata/mips Git branch with a patch called Support
 tracing of PLT-less MIPS binaries on top... which has not entered
 master.

So yeah, I spent some time on that (possibly the weekend and then a
couple evenings before 2014-01-21).  Looking into my notes, I needed to
move on to Aarch64 support, and the MIPS work must have gradually
slipped my mind.

It seems like I wanted to redo what PowerPC (and newly Xtensa!) was/is
doing with unresolving PLT slots when they are resolved, so that we
don't have to move breakpoints to function entry points, but can keep
them in PLT.  I'm not sure how far I got.  The code for this
specifically seems to be in, but it is likely that I broke some other
MIPS use cases, special symbol types or other MIPS magic.

Someone would need to fix regressions in test cases between
pmachata/mips and 94773bf0b1 (the branch-off point) and fix them.  They
would need to have enough domain knowledge to realize where the test
suite has blind spots, what it doesn't test, and add tests and fixes for
these test cases.  That someone is unlikely to be me, my personal time
budget is actually even more strained now than it was a year ago.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] xtensa: add xtensa support

[Petr Machata]

Max Filippov jcmvb...@gmail.com writes:

 - drop gimme_arg and implement arch_fetch_* callbacks;

Cool.

 - add diagnostics to {get,set}_instruction_pointer, get_stack_pointer
   and get_return_addr;
 - update CREDITS, NEWS and README.

 Changes RFC-PATCH:
 - adopt PLT unresolving algorithm used by PPC

Also cool.

 +int
 +arch_elf_init(struct ltelf *lte, struct library *lib)
 +{
 + Elf_Scn *scn;
 + GElf_Shdr shdr;
 + GElf_Addr relplt_addr;
 + GElf_Xword relplt_size;
 + GElf_Phdr phdr;
 + size_t i;
 +
 + for (i = 0; gelf_getphdr(lte-elf, i, phdr) != NULL; ++i) {
 + if (phdr.p_type == PT_LOAD) {
 + lib-arch.loadable_sz =
 + phdr.p_vaddr + phdr.p_memsz - lte-bias;

Was this tested on a prelinked system?  It's confusing that you subtract
bias to get from ELF address to memory address.  Normally you would add
it.

Also, correct me if I'm wrong, but loadable_sz actually points to the
end of the region, and hence is not a size.  Would loadable_end be a
better name?

 + if (elf_get_section_type(lte, SHT_DYNAMIC, scn, shdr)  0
 + || scn == NULL) {
 + fail:
 + error(0, 0, Couldn't get SHT_DYNAMIC: %s,
 +   elf_errmsg(-1));
 + return -1;
 + }
 +
 + Elf_Data *data = elf_loaddata(scn, shdr);
 + if (data == NULL)
 + goto fail;
 +
 + for (i = 0; i  shdr.sh_size / shdr.sh_entsize; ++i) {
 + GElf_Dyn dyn;
 +
 + if (gelf_getdyn(data, i, dyn) == NULL)
 + goto fail;
 +
 + if (dyn.d_tag == DT_JMPREL) {
 + relplt_addr = dyn.d_un.d_ptr;
 + } else if (dyn.d_tag == DT_PLTRELSZ) {
 + relplt_size = dyn.d_un.d_val;
 + }
 + }

Would calling elf_load_dynamic_entry twice be suitable instead of the
above block?

The two calls would end up looking up the section by type twice.  That
has negative performance implications.  I suspect they are negligible,
but should you care, would you be also willing to contribute a patch
that implements on-demand caching of SHT_DYNAMIC inside
elf_load_dynamic_entry?

 + for (i = 1; i  lte-ehdr.e_shnum; ++i) {
 + Elf_Scn *scn;
 + GElf_Shdr shdr;
 +
 + scn = elf_getscn(lte-elf, i);
 + if (scn == NULL || gelf_getshdr(scn, shdr) == NULL) {
 + fprintf(stderr, Couldn't get section header: %s\n,
 + elf_errmsg(-1));
 + exit(EXIT_FAILURE);
 + }
 + if (shdr.sh_addr == relplt_addr
 +  shdr.sh_size == relplt_size) {

To get the section pointed to by DT_JMPREL, you could call
elf_get_section_covering and then check the size in the returned shdr.
My thinking is that if there is more than one section covering a given
address, the ELF is probably not structurally sound, and you can safely
bail out anyway.

FWIW, the ARM backend simply does this:

GElf_Addr jmprel_addr;
Elf_Scn *jmprel_sec;
GElf_Shdr jmprel_shdr;
if (elf_load_dynamic_entry(lte, DT_JMPREL, jmprel_addr)  0
|| elf_get_section_covering(lte, jmprel_addr,
jmprel_sec, jmprel_shdr)  0
|| jmprel_sec == NULL)
return -1;

Other than these things, the code looks solid as far as I can tell (not
that I know anything about xtensa though ;) ).

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH v2] xtensa: add xtensa support

[Petr Machata]

Max Filippov jcmvb...@gmail.com writes:

 On Tue, Dec 9, 2014 at 2:34 AM, Max Filippov jcmvb...@gmail.com wrote:
 Signed-off-by: Max Filippov jcmvb...@gmail.com
 ---
 Changes v1-v2:
 - drop gimme_arg and implement arch_fetch_* callbacks;
 - add diagnostics to {get,set}_instruction_pointer, get_stack_pointer
   and get_return_addr;
 - update CREDITS, NEWS and README.

 Ping?
 Petr, I see that this letter haven't made it to the mailing list because
 of its size. Do I need to re-post it?

Depends on you.  I can review and approve it either way.

Sorry I didn't get to it yet, I'm on vacation now and things have been
creazy around releasing 0.1 of dwgrep before I left for vacation.

Thanks,
Petr

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Compilation error with ltrace head

[Petr Machata]

Thierry fa...@linux.vnet.ibm.com thie...@linux.vnet.ibm.com writes:

 In file included from library.h:32:0,
  from library.c:30:
 library.c: In function 'library_dump_symbols':
 dict.h:223:25: error: passing argument 1 of 'dict_each' discards
 const' qualifier from pointer target type [-Werror]
(KEY_TYPE *)dict_each((DICTP), _start_after,  \

Sorry about that.  I reverted the commit that introduced the functions
that cause this build failure.

 Do I need to check on ppc64 the status ?

Please re-check.  I changed some relevant code in eea609 and following
commits.  I'm almost sure the test suite was clean after I was done with
it (which it didn't before BTW).  I'm pretty sure that I tested plain
ppc64, ppc32 with BSS PLT's and ppc32 with secure PLT's as well.  It all
worked.  But please re-check, more eyeballs and all that.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] DWARF test suite

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 At the end of dwarf.exp I was looking for

  ft...@libc.so.6(0x[0-9a-z]*) * = -1

 Note the ' * ' with 1 space on either side of *. In your tree you
 changed it to '  * '. This is in two places at the end of that file.

Curious.  The rest of whitespace tweak to DWARF test suite entry
clearly applied, dunno why this bit didn't.

 Other than that, it looks fine. Thanks

OK, I pushed it now.

P.

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] DWARF test suite

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 Hi. I just uploaded a patch that does the filename-language-choice as
 you suggested:

 .c11  is C 11
 .cc11 is C++ 11
 .cc, .cpp is C++ (whatever gcc has as the default)

 This choice is still in ltrace.exp. The DWARF-specific piece simply
 renames its .cc - .cc11

Cool, that looks good.

There was a number of commits and fixes of commits on your branch, then
the commit that renamed .cc to .cc11 depended on a newer commit that
introduced that support to ltrace.exp, and forgot to project the change
in Makefile.am as well.  So I reworked the branch into a couple pieces
that I feel belong together and fixed those minor issues.  It's on
pmachata/dima-rebase.  Can you please take a look whether I haven't
forgotten about anything?

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Restricting functions printed by ltrace

[Petr Machata]

Reposting with Victor's permission:

Victor Porton por...@narod.ru writes:

 12.08.2014, 11:12, Petr Machata pmach...@redhat.com:
 Victor Porton por...@narod.ru writes:
  I use

  ltrace -n4 -A10 -llibraptor2.so.0 ./obj/test/debug/run_all_tests 21| 
 egrep ^[a-z]

  to output only these calls which happen from my program
 run_all_tests not other functions of libraptor2.

  This use of grep is silly. There should be a command line option of
  ltrace which would limit only to functions called from the user
  supplied program (not from other parts of the traced library), or more
  generally limit to N levels of indirection (by indirection I mean the
  same thing which directs indentation when -n4 or a similar option is
  supplied).

 I agree, this sort of filtering shoud be doable on selector level.
 I don't think we cover this right now.  -e allows this sort of
 fine-grained selection, but you can't say
 -e symbols that come from libfoo.so@myprogram.

My current thinking is that we could extend -e to something like
[soname_pattern:][symbol_pattern][@soname_pattern] (i.e. add the initial
optional soname selector).  -l libfoo.so would become syntax sugar for
-e libfoo.so:*@*.

This would require merging the logic for our PLT handling and export
handling.  Symbols that match the *@* part, but not the *: part would be
made latent.  Later if a library is mapped that matches the *:, those
symbols would be activated.

(Actually the logic would be a bit more complex.  When a library is
mapped that matches the *: part of some rule, we look up libraries that
match the *@ part of the same rule, and in those, enable the symbols
that match the * part of that rule.  We can't just whole-sale enable
everything latent that the newly-mapped library brings in, as that would
break cases like -e libfoo.so:x*@A -e libbar.so:y*@A.  If libfoo has
symbols x1 and y1, and libbar has x2 and y2, and all of these are called
from A, we only wish to enable x1 when libfoo is mapped, and never x2
when libbar is mapped; and vice versa--only enable y2 when libbar is
mapped, and never y1 when libfoo is mapped.)

Filtering based on -n distance is another feature request, and I don't
really have any thoughts on this.

Tell me what you think.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Add support for using elfutils as unwinder.

[Petr Machata]

Andreas Schwab sch...@linux-m68k.org writes:

 It would, if it would apply.

How about this?

diff --git a/dwarf_prototypes.c b/dwarf_prototypes.c
index 9c36904..a39219e 100644
--- a/dwarf_prototypes.c
+++ b/dwarf_prototypes.c
@@ -8,6 +8,7 @@
 #include stdio.h
 #include elfutils/libdwfl.h
 #include dwarf.h
+#include inttypes.h
 #include stdlib.h
 #include errno.h
 #include string.h
@@ -27,7 +28,7 @@
 #include debug.h
 
 #define complain(die, format, ...) \
-   debug(DEBUG_FUNCTION, %s() die '%s' @ 0x%lx:  format, \
+   debug(DEBUG_FUNCTION, %s() die '%s' @ 0x% PRIx64 :  format, \
  __func__, dwarf_diename(die), dwarf_dieoffset(die),   \
  ##__VA_ARGS__)
 

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Compilation error with dwarf code

[Petr Machata]

Kai Noda noda...@gmail.com writes:

 Kai Noda noda...@gmail.com writes:

  +#if defined(HAVE_LIBDW)
   #include dwarf_prototypes.h
  +#endif /* defined(HAVE_LIBDW) */

 I don't think these two lines constitute a creative work protected by
 copyright laws.  No authorship/copyright claimed.

OK, applied.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] PPC: fix PPC32 build

[Petr Machata]

Pushed.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Config files

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 Hi all.

 In putting together the test suite, I hit a few issues with conf files.
 Those are all described together in this email.

 When running the test suite, I would think we want to be testing the
 build of ltrace that appears in the checked-out source tree, and that it
 should be independent of the user's local configuration or the overall
 system. Thus I would expect

 - ~/.ltrace to not be read
 - /etc/ltrace to not be read either
 - etc/ in the source tree to be read

Agreed.  I never got around to implementing this, but it's desirable.
There's a test case that fails if you run ltrace built with
pkgdatadir=/etc.  I know to look for these, but others naturally don't,
and it causes some trouble now and then.

My thinking was to introduce a --sysroot, akin to what gcc and ld have.
For testing, this could be set to source directory, or whatever.

I don't know how to cleanly disable reading of user config files.
Environment variable?  Another flag?  Some -F mojo?

 currently I'm observing that

 - ~/.ltrace is read
 - /etc/ltrace is not read

This might be because /usr/local/etc/ltrace is attempted and not found.

 - etc/ in the source tree is not read

 This sounds wrong. Complaints against patching it in this way?

Depends on how exactly you want to have it patched ;) The sysroot
approach is somewhat appealing to me, but only solves half of the
problem.  Maybe you have something better in mind.

 The DWARF parser interprets every structure as deeply as possible. This
 is often undesirable. For instance when tracing something like ftell(),
 ltrace wants to print all components of its FILE* argument. On my libc,
 this eventually contains a void value, and ltrace does an assert(0)
 trying to print it. We want to ship something like

I wonder if that's a bug in ltrace.  Either claiming that a value is of
type void (what does that mean, exactly?), or else, if there are
legitimate reasons (void pointers and return types aside), that it
asserts on them.

  typedef FILE = addr;

 in our config files. We DO have this in libc.so.conf, but this has 2
 problems:

 1. this file isn't read by the test suite, as mentioned above

 2. On Debian, the DWARF data has ftell(_IO_FILE*), not ftell(FILE*). It
 also has typedef FILE = _IO_FILE. Thus the typedef in the conf file has
 no effect. We can add to the conf file

  typedef _IO_FILE = FILE

Just to be clear, the reason for this typedef is backward compatibility.
ltrace used to recognize FILE as a bulit-in type, but it never did
anything useful with it.  When I rewrote type handling, I decided to
drop it and just exposed it as a typedef of addr.

 but this is an implementation detail of glibc, so this is unideal. We
 can also do something complicated like realizing that FILE is defined in
 the conf file, and then treat the typedef in the DWARF backwards.
 Suggestions?

What do you mean, backwards?

The useful way of handling FILE would probably be to pry interesting
data from it and display that.  Like, file descriptor if any, stream
position, that sort of thing.  This sort of thing is already hard to
write with the current, um, scripting language in ltrace, and it's
pretty much impossible unless we can distinguish one glibc from
another.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] ltrace not working on some shared objects

[Petr Machata]

Patric Schmitz bzk0...@aol.com writes:

 that easily, so I'm kinda stuck here. What might be reasons for ltrace
 not being able to trace calls in-between linked shared objects?

ltrace doesn't show calls from shared libraries by default, but you can
configure that it does.  You could just say -e \*, which should trace
everything, but that tends to slow the applications too much.  You can
say something like -e '*@libsomething*' to trace calls from
libsomething.so.X.

In your case, if you want to trace calls _to_ libGL, the more useful
ltrace flag might be -l libGL\*.

Do specify -f for multithreaded objects.

Also, the above hasn't worked the way I describe it until ltrace = 0.7.
Not sure where your ltrace comes from, some distros carry obsolete
versions.  Current GIT master is in a fairly good shape and should work.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] DWARF prototypes: handling symbol aliases

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 I fixed this, and the tree now has another patch.

I merged your code.

I also fixed a bug where lib-dwfl_module was left uninitialized on
clone.  main-vfork.exp was failing on that (in fact it has been like
that for some time now).

 This somewhat makes sense: the nanosleep being called in this case lives
 in libpthread.so, not in libc6.so; and for some reason in Debian the
 debug symbols of libpthread.so have nothing about nanosleep (or
 __nanosleep, etc). But if this is so, then why does it work with -e and
 -l? Does it make sense to you? In any case, this wrinkle probably is
 independent of my changes, and it'd be great if you reviewed the tree.

Prototype lookup for PLT symbols in prototype libraries works on
assumption that name collisions don't happen.  If two libraries export
the same symbol, and each of them has a different prototype, then that's
a bug in the application.  So for -e, you just look into all prototype
libaries opened for the process.  For -x you still look into the
prototype library corresponding to the library itself.

Thanks,
P.

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] ltrace not working on some shared objects

[Petr Machata]

Patric Schmitz bzk0...@aol.com writes:

 On Fri, 25 Jul 2014 11:03:36 +0200
 Petr Machata pmach...@redhat.com wrote:
 Patric Schmitz bzk0...@aol.com writes:
  that easily, so I'm kinda stuck here. What might be reasons for ltrace
  not being able to trace calls in-between linked shared objects?
 
 ... 
 In your case, if you want to trace calls _to_ libGL, the more useful
 ltrace flag might be -l libGL\*.

 I already tried that, but it works only if I link the libraries which
 call into libGL statically. With other applications, e.g. glxgears, I
 can see the calls *from* the shared libGL *to* libnvidia-glcore by
 doing ltrace -l libnv\* glxgears as expected. I need to see the calls
 from my shared objects (which were built as shown in the last mail) to
 libGL.

OK, that's suspicious.  I don't know the exact scenario, but it should
work:

$ cat lib3.c
int lib3() { return 7; }
$ cat lib2.cc
extern C int lib3();
int lib2 () { return lib3 () + 1; }
$ cat main.cc
int lib2 ();
int main(int argc, char *argv[]) { return lib2 () + 1; }
$ gcc -fpic -shared lib3.c -o lib3.so
$ c++ -Wall -Wno-reorder -g -DDEBUG -fPIC -c lib2.cc
$ c++ -fPIC -Wall -Wno-reorder -g -DDEBUG -shared -Wl,-soname,lib2.so  -o 
lib2.so lib2.o -lrt
$ c++ main.cc lib2.so lib3.so 
$ LD_LIBRARY_PATH=. ltrace -l lib3\* ./a.out
lib2.so-lib3(1, 0x7fffc92814b8, 0x7fffc92814c8, 64) = 7
+++ exited (status 9) +++

I'll need a bit more context here.  Can you create a self-contained
example that exhibits this problem?  Something that I can use to debug
whatever is happening?

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] xtensa: add xtensa support

[Petr Machata]

This looks good.  I'm ready to merge this code-wise.  But please mention
xtensa support in README and NEWS, and if you care, add yourself to
CREDITS (the list is alphabetical by last name).  Sending these
additions as an additional patch is fine by me, depends on what you
prefer.

Max Filippov jcmvb...@gmail.com writes:

 +long gimme_arg(enum tof type, struct process *proc, int arg_num)

This interface is fairly limited.  I see[1] that xtensa supports passing
some 64-bit arguments in pairs of registers, that just won't work here.
I think ltrace currently aborts if an attempt is made to call gimme_arg
with a data type that wouldn't fit into long.

ltrace has a new set of interfaces to address the limitations of
gimme_arg.  Those are arch_fetch_* family of callbacks, documented in
fetch.h.  This may all be moot, I'm not familiar with xtensa, so it's up
to you whether you want to write this code.  As stated above, I'm ready
to merge what you have now.

[1] http://wiki.linux-xtensa.org/index.php/ABI_Interface

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [RFC] add xtensa architecture support to ltrace

[Petr Machata]

Max Filippov jcmvb...@gmail.com writes:

 It's an RFC because currently there's an issue I'm not sure how to deal with:
 code blocks that correspond to PLT entries on other architectures are only
 used at most once on xtensa, after initial resolution of imported symbol
 address importing module calls it directly.

I don't know the details of operation of xtensa (and I didn't look into
your code yet), but these sorts of things can be generally solved as
follows.

- Put a breakpoint to PLT, catching the first call, and then stop all
  threads and singlestep through the dynamic linker, and waiting for the
  relevant token (such as GOT entry or whatever) to be updated.  Then
  you start the threads again.

- From the updated token, you figure out what the address is where the
  symbol got resolved (final address below), and store it at struct
  breakpoint.arch.  Then you undo what the dynamic linker did
  (unresolve the token).

- On every next call, when you already know the destination address you
  just change PC to that address via ptrace right away, and let the
  process continue.  You do this after the first call as well.

- If the binary was prelinked, or if you attach to a running process,
  you need to unresolve any hitherto-resolved tokens.

This is fairly involved, but PowerPC backend does this, with some added
twists.  There's however a fair amount of wiggle-room depending on what
you are willing to compromise:

- Instead of single-stepping through dynamic linker, you can simply
  install an on_hit handler to a return breakpoint, figure out the final
  address after return, and then undo it and proceed as above.  The
  downside is that then there's a window between the call and a return,
  where ltrace will miss calls to the same symbol (be they calls from
  other threads or reentrant calls through the same PLT from the same
  thread).

- Or yet simpler, do the on_return thing from previous paragraph, figure
  out the final address, and then just relocate the breakpoint there.
  In addition to the disadvantage above, this one also can't distinguish
  local calls from PLT ones (except for the first one).  If I recall
  correctly, this is what the MIPS backend does.

If you want to implement the full thing, then perhaps looking into
sysdeps/linux-gnu/ppc/plt.c could provide some insight, though with four
ABI's to support, there's a lot of code, and disentangling the relevant
bits may be challenging.  The relevant functions are ppc_plt_bp_continue
and cb_keep_stepping_p.  There's also an extensive comment at the top of
the file explaining some of the complexities that the PowerPC backend
needs to handle and how it does that, but I'm not sure it's enlightening
for anyone working on a non-PowerPC architecture.

The stopping and singlestepping thing is handled by
process_install_stopping_handler.  You call this in PLT breakpoint's
on_continue handler, and provide a keep_stepping_p predicate that
returns CBS_CONT until the dynamic linker updated the relevant token
(then it return CBS_STOP).  You also need to note the final address
somewhere (presumably in breakpoint.arch), and possibly mark a flag in
the breakpoint, so that next time you get to on_continue, you know
you've already done all this, and can just jump to the known final
address right away.

Hopefully this was helpful.  Don't be afraid to ask if you need
anything.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Addition to the manpage

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 OK. New patch pushed up.

Merged.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Addition to the manpage

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 Do you think this is better? I can go either way.

Well, this only adds the exited messages, and I find it better that
you don't hide anything from the user.  The added exited is no
distraction at all, I think.

 I would also put the whole example in a section of its own, presumably
 named EXAMPLE, or maybe SYMBOL FILTERING EXAMPLE.  That should only
 come after the FILTERING EXPRESSIONS, as conceptually it builds on
 what's written there.  It would however be good to refer to this new
 section from -e, -x and -l.

 Sure? I think the way it is now is better. Both putting the examples
 first (all perl docs do that, and they tend to be very good) and putting
 them into the same section (but different subsections). If you don't
 have strong feelings here, I'd rather leave it.

In my opinion, explanations of differences between -x, -e and -l don't
belong in a section that describes filtering expressions, which is a
logically lower-level topic.  I don't have particularly strong feelings
about ordering though, so if you think it's better to put examples
first, then so be it.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] DWARF prototypes: handling symbol aliases

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 Hi. This code wasn't pushed anywhere, but if you want to look at it,
 it's here:

  https://github.com/dkogan/ltrace/tree/libsym_aliases_inexportlist

Took a look now.  Looks reasonable.  The iterator interfaces that you
have seem unlike what's in ltrace (look in callback.h), but they might
be just unfinished.

I'm wondering whether we should stop pretending it's an export list, and
start calling it symtab or something, now that we will unconditionally
construct anyway, and now that we hold more than just names in there.

But anyway.  The approach seems reasonable to me.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH V04 ] Support for ppc64le (Little Endian and ABIv2)

[Petr Machata]

Applied.

Thanks for following through on this,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Support for ppc64le (Little Endian and ABIv2) Add new testsuite/ltrace.main/parameters-str.exp file Handles HFA and irelative support

[Petr Machata]

Thierry Fauck ( thierry @ linux.vnet.ibm.com )
thie...@linux.vnet.ibm.com writes:

 +#ifndef EF_PPC64_ABI
 +# define EF_PPC64_ABI 3
 +#endif
 Shouldn't I had this defintion is arch.h when I am not in case
 _CALL_ELF==2 so when I want to use it I can bail out if EF_PPC_ABI
 undefined in arch_elf_init() when I want to use it.

Yeah, makes sense.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Mark Wielaard m...@redhat.com writes:

 On Sun, 2014-05-11 at 23:12 +0200, Petr Machata wrote:
 My mistake, I thought we have a Dwfl per process, but we have one per
 library.  Having it stored at process would perhaps fit better with the
 overall design of Dwfl, but I guess there's no bug as things stand now.

 We do have one Dwfl per process for the unwinder support. It is in
 struct process and setup once for each leader in proc_add_library if it
 doesn't exist yet and if it already exists then proc_add_library will
 just add the new Elf to the Dwfl. As far as I understand the new code
 this Dwfl is then set as the dwfl of struct library too.

I see it now.  So correct me if I'm wrong, but that means that after the
first dwfl_report_elf, dwfl_nextcu would iterate over just that one
module.  However after second dwfl_report_elf, dwfl_nextcu would again
iterate over the first module, as well as the second module.

If that's correct, then I believe what we should store at struct library
is Dwfl_Module, not Dwfl itself.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Mark Wielaard m...@redhat.com writes:

 On Mon, 2014-05-12 at 16:36 +0200, Petr Machata wrote:
 I see it now.  So correct me if I'm wrong, but that means that after the
 first dwfl_report_elf, dwfl_nextcu would iterate over just that one
 module. However after second dwfl_report_elf, dwfl_nextcu would again
 iterate over the first module, as well as the second module.
 
 If that's correct, then I believe what we should store at struct library
 is Dwfl_Module, not Dwfl itself.

 Yes, that seems correct. dwfl_report_elf does report the Dwfl_Module (or
 NULL on failure). So you can use that and dwfl_module_nextcu.

Dima, I think you should be able to verify whether the second
dwfl_nextcu iterates over the first module's data as well.  While that's
probably harmless, it's a work duplication, and a better design would be
to store Dwfl_Module at struct library and use dwfl_module_nextcu as
indicated above.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Support for ppc64le (Little Endian and ABIv2) Add new testsuite/ltrace.main/parameters-str.exp file Handles HFA and irelative support

[Petr Machata]

Thierry, will you be able to address these last nits?  It's the last
hump before I can merge this without breaking compilation on hosts that
don't know about ELFv2 ABI yet.

Thank you,
PM

Petr Machata pmach...@redhat.com writes:

 This looks all pretty good.  I compile-tested it on plain PPC64 --it's
 desirable to keep ltrace building on old toolchains as well-- and hit a
 couple problems with the several new symbols that you use.  I think we
 should have a fallback that disables the new code if the definitions are
 just unavailable.

 diff --git a/sysdeps/linux-gnu/ppc/fetch.c b/sysdeps/linux-gnu/ppc/fetch.c
 index ed38336..b42cd95 100644
 --- a/sysdeps/linux-gnu/ppc/fetch.c
 +++ b/sysdeps/linux-gnu/ppc/fetch.c
 [...]
  
 +static int
 +get_return_info(struct arg_type_info *info, struct process *proc,
 +struct fetch_context *context)

 For non-ELFv2 host, I'm getting:
 sysdeps/linux-gnu/ppc/fetch.c:119:1: error: 'get_return_info' defined but not 
 used [-Werror=unused-function]
 sysdeps/linux-gnu/ppc/fetch.c:392:1: error: 'allocate_hfa' defined but not 
 used [-Werror=unused-function]

 diff --git a/sysdeps/linux-gnu/ppc/plt.c b/sysdeps/linux-gnu/ppc/plt.c
 index 332daa8..a16e182 100644
 --- a/sysdeps/linux-gnu/ppc/plt.c
 +++ b/sysdeps/linux-gnu/ppc/plt.c
 [...]
 @@ -430,7 +448,12 @@ reloc_copy_if_irelative(GElf_Rela *rela, void *data)
  int
  arch_elf_init(struct ltelf *lte, struct library *lib)
  {
 +
 +/* Check for ABIv2 in ELF header processor specific flag.  */
 +lte-arch.elfv2_abi = ((lte-ehdr.e_flags  EF_PPC64_ABI) == 2);
 +

 So, I don't think we should turn off detection.  It seems appropriate to
 me to do something like:

 +#ifndef EF_PPC64_ABI
 +# define EF_PPC64_ABI 3
 +#endif

 somewhere around here (or possibly at the top of this file).  If
 EF_PPC64_ABI is undefined, and we detect elfv2_abi, we should bail out
 with a message about unsupported ABI.

 @@ -629,9 +652,45 @@ arch_elf_add_func_entry(struct process *proc, struct 
 ltelf *lte,
  arch_addr_t addr, const char *name,
  struct library_symbol **ret)
  {
 -if (lte-ehdr.e_machine != EM_PPC || lte-ehdr.e_type == ET_DYN)
 +/* With ABIv2 st_other field contains an offset.  */
 + if (lte-arch.elfv2_abi)
 +addr += PPC64_LOCAL_ENTRY_OFFSET(sym-st_other);

 Similar problem.  This should be ifdef'd away I think.  I.e. something
 like:

 #ifndef PPC64_LOCAL_ENTRY_OFFSET
 assert(! lte-arch.elfv2_abi);
 #else
 if (lte-arch.elfv2_abi)
 addr += ...
 #endif

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 This doesn't seem right.  dwfl_nextcu should go through all CU's of the
 whole Dwfl--i.e. if you add two Dwarf-based libraries to a process, it
 would iterate the former one twice.  Or maybe I don't quite grasp how
 your code works.

 I'm not trying to do anything noteworthy here; just iterating through
 all the compile units. You're saying that if a process that uses two
 libraries, this could would look through those libraries twice? I don't
 understand the issue you're describing.

My mistake, I thought we have a Dwfl per process, but we have one per
library.  Having it stored at process would perhaps fit better with the
overall design of Dwfl, but I guess there's no bug as things stand now.

 Is there a reason not to just use uint64_t for encoding?

 Nope. Typo.

Then this doesn't need the cast ;)

@@ -200,7 +200,7 @@ static bool get_integer_base_type(enum arg_type *type, int 
byte_size,
 // support a particular type (or an error occurred), I regturn ARGTYPE_VOID
 static enum arg_type get_base_type(Dwarf_Die *die)
 {
-   int64_t encoding;
+   uint64_t encoding;
if (!get_die_numeric((uint64_t*)encoding, die, DW_AT_encoding))
return ARGTYPE_VOID;

But that's a detail.

One last thing.  Can you please:

$ git remote add debian git://git.debian.org/git/collab-maint/ltrace.git
$ git fetch debian
$ git rebase debian/master
$ git push -f

... so that I can merge cleanly, without the merge commit?

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 Rebased.

OK, merged.  I forgot about one more thing: please add a NEWS item for
your change and, if you care, a CREDITS entry for yourself as well.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 I looked into this, and it's caused by the thread. If you modify your
 main() in hle1.c to do pthread_create() and call jedna() from the
 thread, then the in-second-thread jedna() call is shown with the default
 prototype. This is with 'ltrace -f -l'.

Ah, you're right, I've got it reproduced now.  The following fixlet
takes care of this problem:

diff --git a/output.c b/output.c
index 0cec653..671a5d7 100644
--- a/output.c
+++ b/output.c
@@ -531,7 +531,7 @@ output_left(enum tof type, struct process *proc,
 
account_output(current_column, fprintf(options.output, ());
 
-   struct prototype *func = lookup_symbol_prototype(proc, libsym);
+   struct prototype *func = lookup_symbol_prototype(proc-leader, libsym);
if (func == NULL) {
fail:
account_output(current_column, fprintf(options.output, ???));

It's now on master (together with a test case).

Thank you,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Support for ppc64le (Little Endian and ABIv2) Add new testsuite/ltrace.main/parameters-str.exp file Handles HFA and irelative support

[Petr Machata]

This looks all pretty good.  I compile-tested it on plain PPC64 --it's
desirable to keep ltrace building on old toolchains as well-- and hit a
couple problems with the several new symbols that you use.  I think we
should have a fallback that disables the new code if the definitions are
just unavailable.

 diff --git a/sysdeps/linux-gnu/ppc/fetch.c b/sysdeps/linux-gnu/ppc/fetch.c
 index ed38336..b42cd95 100644
 --- a/sysdeps/linux-gnu/ppc/fetch.c
 +++ b/sysdeps/linux-gnu/ppc/fetch.c
[...]
  
 +static int
 +get_return_info(struct arg_type_info *info, struct process *proc,
 + struct fetch_context *context)

For non-ELFv2 host, I'm getting:
sysdeps/linux-gnu/ppc/fetch.c:119:1: error: 'get_return_info' defined but not 
used [-Werror=unused-function]
sysdeps/linux-gnu/ppc/fetch.c:392:1: error: 'allocate_hfa' defined but not used 
[-Werror=unused-function]

 diff --git a/sysdeps/linux-gnu/ppc/plt.c b/sysdeps/linux-gnu/ppc/plt.c
 index 332daa8..a16e182 100644
 --- a/sysdeps/linux-gnu/ppc/plt.c
 +++ b/sysdeps/linux-gnu/ppc/plt.c
[...]
 @@ -430,7 +448,12 @@ reloc_copy_if_irelative(GElf_Rela *rela, void *data)
  int
  arch_elf_init(struct ltelf *lte, struct library *lib)
  {
 +
 + /* Check for ABIv2 in ELF header processor specific flag.  */
 + lte-arch.elfv2_abi = ((lte-ehdr.e_flags  EF_PPC64_ABI) == 2);
 +

So, I don't think we should turn off detection.  It seems appropriate to
me to do something like:

+#ifndef EF_PPC64_ABI
+# define EF_PPC64_ABI 3
+#endif

somewhere around here (or possibly at the top of this file).  If
EF_PPC64_ABI is undefined, and we detect elfv2_abi, we should bail out
with a message about unsupported ABI.

 @@ -629,9 +652,45 @@ arch_elf_add_func_entry(struct process *proc, struct 
 ltelf *lte,
   arch_addr_t addr, const char *name,
   struct library_symbol **ret)
  {
 - if (lte-ehdr.e_machine != EM_PPC || lte-ehdr.e_type == ET_DYN)
 + /* With ABIv2 st_other field contains an offset.  */
 +  if (lte-arch.elfv2_abi)
 + addr += PPC64_LOCAL_ENTRY_OFFSET(sym-st_other);

Similar problem.  This should be ifdef'd away I think.  I.e. something
like:

#ifndef PPC64_LOCAL_ENTRY_OFFSET
assert(! lte-arch.elfv2_abi);
#else
if (lte-arch.elfv2_abi)
addr += ...
#endif

Thank you,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 Petr Machata pmach...@redhat.com writes:

 So this is the result of my advice to only look to immediate protolib
 when looking up prototypes.  It seems you need to look recursively after
 all.  (Which should just mean passing true to
 protolib_lookup_prototypes.)

 Passing true to protolib_lookup_type() makes it work the way one would
 expect. Passing true to protolib_lookup_prototypes() doesn't make it
 work. This isn't surprising, I think. Should I commit this change

No it isn't, I mixed them up.

 (passing true to both), or was this just a suggestion for testing? I'm

The change involving protolib_lookup_type(plib, name, true) looks right
to me, and if it fixes the problem, so much the better.  This would be
nice to have as a separate patch, as it's an isolated fix independent of
your larger work.

 a bit concerned about this treating the type namespace as global, even
 though it isn't. What would happen if two different DSOs define an
 identically-named type that isn't actually the same? Can looking for
 exports get confused by this?

Passing true to protolib_lookup_{proto,}type just means to look through
imports in addition to the immediate prototype library, not to other
prototype libraries sa well.  So they are not global per se.

But whatever is in .ltrace.conf is global, so that a user can put in
whatever they want and just have it appear in appropriate places.

About the clashes in general... note that when looking for a prototype
of a PLT call, we do look through all the prototype libraries in the
process, so in a sense, the namespace _is_ global.  Then again, any
exported symbol better be defined in one place only.

  void FILEtest( FILE* a, const FILE* b );

 A lens that does what I want is

  void FILEtest( void*, void* );

(BTW, this is a prototype, not a lens.  A lens is a way to look at a
piece of data.  So hex or enum are lenses, because they take the raw
underlying integer, and format it in interesting ways.  Similarly a
string is a lens, because it formats pointers or arrays in interesting
ways.)

 But I'd like to achieve this with some sort of lens typedef on FILE. Is
 this possible? Or more to the point, what would be the most useful

typedef void FILE; should work.

 ltrace output for a FILEtest() call, and how should it be implemented?

The most useful way would be to show a file name, I guess.  ltrace.conf
is not expressive enough to code this logic, but it can't always be done
anyway, for streams opened by fdopen.

So the next best thing is the file descriptor.  But that can't always be
done either, I suspect, for streams returned by fmemopen and the like.

That's the reason I just keep it as a void*, the pointer ends up being
the most meaningful identifier of each stream, even though the directory
counterpart DIR actually shows the file descriptor.

 I think this all works sufficiently now for me to get the details ironed
 out (memory, error handling), so I'm going to be looking at that next.

Cool.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Mark Wielaard m...@redhat.com writes:

 On Thu, 2014-04-24 at 01:48 +0200, Petr Machata wrote:
 You're right, I'm not sure what I was looking at.  I don't see it as
 udata now either.

 Maybe you were using a trunk GCC compiler?

Nope, pretty sure it was 4.8.2.  Must have been some sort of thinko, I'm
not sure what I was looking at.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Quick question about conf file

[Petr Machata]

Thierry fa...@linux.vnet.ibm.com thie...@linux.vnet.ibm.com writes:

 I get a corner case where a function returns a struct of string  16
 Bytes, I get an optimization.
  
 What should be the conf file to have in the info structure the size of
 the string ?
 is
 struct(array(char,16)) struct_func_str16(struct(string[16]));

 the best defintion, knowing that the output of the array won't be
 truncated and I would check for the 16 chars ?

I'm not sure what you are asking for.

struct(string(array(char, 16))) is how I think a return of a buffer of
static 16 chars should be returned.  Technically ltrace conf file should
allow string(array(char,16)) as the return type as well, but there might
be bugs in support of naked arrays.

For determining the size of a arg_type_info, use type_sizeof.

 If I say struct(string[16]) struct_func_str16(struct(string[16]));
 I don't have the proper size if the size is  17

I think string[16] expands to string(array(char, 16)*), i.e. what in C
would be struct{char *buf;}, not struct{char buf[16];}.

I don't know what you mean by the size  17.  The return type must be
compile-time static, I very much doubt that return blah in C just
happens to pass the return value in different registers than return
blahblahblahblah (a 17-character string).

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Dima Kogan li...@dima.secretsauce.net writes:

 - added a disabled stub for complex float support. ltrace itself doesn't
   have complex lenses yet. I'd like to get the core of this new DWARF
   functionality working, then build extra things on it. Thus I'd like to
   punt on this one until later

Sure, no problem.  I believe all ABI's essentially treat complex as a
struct of two floating point values, so the support is present, even
though we don't have a lens to format these in a fancy manner.

 I looked into this, and GCC currently doesn't seem to emit DW_AT_type
 for enums with specified base.  Instead it denotes signedness by
 encoding enumerators with DW_FORM_udata or DW_FORM_sdata.  Size of the
 underlying type is encoded in DW_TAG_enumeration_type's DW_AT_byte_size,
 which might be important for enums in structs (I don't think it will
 matter in naked parameters, unless in weird cases like __int128, which
 ltrace doesn't handle anyway).

 I ran some tests. Indeed, there's no DW_AT_type, but I always see
 DW_FORM_sdata, even if it's supposedly unsigned. This is gcc 4.8.2. In
 any case, the current code in my tree assumes signed. I think this is
 fine for the first pass.

You're right, I'm not sure what I was looking at.  I don't see it as
udata now either.

 What you end up inserting is an incomplete type.

 This was done intentionally to support recursive types. Let's say you
 have struct S { struct S* s; };. The code

[...]

 I THINK (correct me if I'm wrong) that if implemented the way you
 suggest, the getType() call would never complete.

Right, I see.  Good catch.

 Later in process_die_compileunit:

 +const char* function_name = dwarf_diename(die);

 This could concievably return NULL for DW_AT_name-less DIE.

 As part of another change I'm no longer reading functions that have no
 name. Makes sense?

It does.  We couldn't assign them a prototype anyway.

 +static bool import( struct protolib* plib, struct library* lib, Dwfl* dwfl )
 +{
 +dict_init(type_hash, sizeof(Dwarf_Off), sizeof(struct arg_type_info*),
 +  dwarf_die_hash, dwarf_die_eq, NULL );

 type_hash should be a local variable in import and the pointer should be
 passed to other functions.  Is there a reason for making it global?

 Only that it makes the code simpler. It's a static global, so its scope
 is limited to dwarf_prototypes.c. In my opinion this is an acceptable
 tradeoff. If you really want it to be local to some function, and pass
 the pointer around instead, tell me and I'll do it that way.

Yes please.

 OK. I did this, but I'm not certain it's right, so please check. In
 library_get_prototype() I now do the import if needed. The
 import_DWARF_prototypes() function itself calls protolib_cache_default()
 to init the protolib, then reads the DWARF data on top of that. Is this
 right? I DO want to read the config files first to have pieces take
 precedence over the DWARF, so overall this should be good, but the
 details are likely wrong.

It's been some time that I wrote this code, so I don't recall details
either.  I'll take a look at your code and let you know.

 -e traces PLT calls (i.e. inter-library calls), -x traces symbol entry
 points.  -l traces PLT calls done to a symbol defined by a library in
 -l.

 Can we add this to the manpage? I don't feel qualified to do this myself
 yet.

How about this?

diff --git a/ltrace.1 b/ltrace.1
index f683844..93032f2 100644
--- a/ltrace.1
+++ b/ltrace.1
@@ -122,11 +122,13 @@ describing which debug messages should be displayed.  Use 
the option
 \-Dh to see what can be used, but note that currently the only
 reliable debugmask is 77, which shows all debug messages.
 .IP \-e \fIfilter
-A qualifying expression which modifies which library calls to trace.
-The format of the filter expression is described in the section
-\fBFILTER EXPRESSIONS\fR.  If more than one \-e option appears on the
-command line, the library calls that match any of them are traced.  If
-no \-e is given, \fB@MAIN\fR is assumed as a default.
+A qualifying expression which modifies which library calls (i.e. calls
+done through PLT slots, which are typically calls from the main binary
+to a library, or inter-library calls) to trace.  The format of the
+filter expression is described in the section \fBFILTER
+EXPRESSIONS\fR.  If more than one \-e option appears on the command
+line, the library calls that match any of them are traced.  If no \-e
+is given, \fB@MAIN\fR is assumed as a default.
 .IP \-f
 Trace child processes as they are created by
 currently traced processes as a result of the fork(2)
@@ -145,6 +147,9 @@ Print the instruction pointer at the time of the library 
call.
 .IP \-l, \-\-library \fIlibrary_pattern
 Display only calls to functions implemented by libraries that match
 .I library_pattern.
+This is as if you specified one \-e for every symbol implemented in a
+library specified by
+.I library_pattern.
 Multiple library patters can be specified 

Re: [Ltrace-devel] Getting prototypes from debug information

[Petr Machata]

Petr Machata pmach...@redhat.com writes:

   What we are using is very close to what Linux kernel coding style
   looks like.

One more thing, opening curly braces should be placed at the end of the
line, except for functions.  (This is the same that Linux kernel does.)

 - in getEnum, you assume that the underlying type is INT.  That may not
   be the case.  In C++11, you can actually choose the underlying enum
   type.  DW_TAG_enumeration_type's DW_AT_type (if present) will point to
   that underlying type.

I looked into this, and GCC currently doesn't seem to emit DW_AT_type
for enums with specified base.  Instead it denotes signedness by
encoding enumerators with DW_FORM_udata or DW_FORM_sdata.  Size of the
underlying type is encoded in DW_TAG_enumeration_type's DW_AT_byte_size,
which might be important for enums in structs (I don't think it will
matter in naked parameters, unless in weird cases like __int128, which
ltrace doesn't handle anyway).

Next, about the memory management.  That's rather spartan.  All the
own values that you pass to all sorts of initializers determine
whether the initialized structure owns the passed-in pointer.  Most of
the time they will, because their lifetimes are generally different, and
you end up cloning stuff anyway.  So the own == 0 case is really only
useful for referencing static data, and is not terribly helpful.

I'm wondering about this idiom, that you use in a couple places:

+   case DW_TAG_pointer_type:
[...]
+   *info = calloc( 1, sizeof(struct arg_type_info) );
[...]
+   type_init_pointer(*info, NULL, 0);
+
+   complain(type_die, Storing pointer type: %p, *info);
+   dict_insert( type_hash, die_offset, info );
+   return getType( (*info)-u.ptr_info.info, next_die );

What you end up inserting is an incomplete type.  If getType later
fails, it will never properly finish initializing info.  Later on, a
DW_TAG_const_type referencing that type will pick it up, incomplete
(presumably) and return a legitimately-looking arg_type_info that will
kill ltrace.  I would prefer it written this way:

+   struct arg_type_info *ptr;
+   if (getType(ptr, next_die)) {
+   type_init_pointer(*info, ptr, 1);
+   complain(type_die, Storing pointer type: %p, *info);
+   dict_insert( type_hash, die_offset, info );
+   return true;
+   } else {
+   free(*info);
+   return false;
+   }

Next, this:

+   case DW_TAG_typedef: ;
+   case DW_TAG_const_type: ;
+   case DW_TAG_volatile_type: ;

Presumably the semicolons here are so that you can declare bool res on
the next line.  Just put the whole thing into scope braces.  You do the
same in attr_numeric.

+   // no type. Use 'void'. Normally I'd think this is 
bogus, but stdio
+   // typedefs something to void
+   *info = type_get_simple( ARGTYPE_VOID );
+   complain(type_die, Storing void type: %p, *info);

Wow, I'll be damned.  DWARF4 actually permits typeless typedefs for
typedef declarations (or forward typedefs if you will--C doesn't have
this).  I don't think it's correct to emit it like that in this case.  I
filed a bug about it:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60885

However this doesn't help us, we'll need to support what's out there.
So yeah, that code should stay in.

+   while(1) {
+   if( dwarf_tag(arg_die) != DW_TAG_formal_parameter )
+   goto next_prototype_argument;
+

I don't think the goto is necessary or very convenient here.  Just
reverse the condition and put what's between the goto and the label into
a block.

+   next_prototype_argument: ;
+   int res = dwarf_siblingof(arg_die, arg_die);
+   if( res == 0 ) continue; /* sibling exists*/
+   if( res  0 )  return false; /* error */
+   break;   /* no sibling exists */

dwarf_siblingof promises to return -1/0/+1.  So:

+   switch (dwarf_siblingof(arg_die, arg_die)) {
+   case 0:
+   /* Sibling exists.  */
+   continue;
+   case -1:
+   /* Error.  */
+   return false;
+   case 1:
+   /* We are done.  */
+   return true;
+   }

Later in process_die_compileunit:

+   const char* function_name = dwarf_diename(die);

This could concievably return NULL for DW_AT_name-less DIE.

+   struct prototype* proto =
+   protolib_lookup_prototype(plib, function_name, 
true );
+
+   if( proto != NULL

Re: [Ltrace-devel] Support for ppc64el patch

[Petr Machata]

Thierry fa...@linux.vnet.ibm.com thie...@linux.vnet.ibm.com writes:

 @@ -34,7 +34,29 @@
  #ifdef __powerpc64__ // Says 'ltrace' is 64 bits, says nothing about target.
  #define LT_ELFCLASS2 ELFCLASS64
  #define LT_ELF_MACHINE2  EM_PPC64
 -#define ARCH_SUPPORTS_OPD
 +
 +#ifdef __LITTLE_ENDIAN__
 +# define BREAKPOINT_VALUE { 0x08, 0x00, 0xe0, 0x7f }
 +# define ARCH_ENDIAN_LITTLE
 +#else
 +# define BREAKPOINT_VALUE { 0x7f, 0xe0, 0x00, 0x08 }
 +# define ARCH_SUPPORTS_OPD
 +# define ARCH_ENDIAN_BIG
 +#ifndef PPC64_LOCAL_ENTRY_OFFSET
 +#define PPC64_LOCAL_ENTRY_OFFSET(other) other
 +#endif

This has wrong indentation.  Please indent a space for each nested
preprocessor level.  (Though I suppose the include guard ifdefs don't
count.)

 @@ -55,14 +57,19 @@ struct fetch_context {
   arch_addr_t stack_pointer;
   int greg;
   int freg;
 - int ret_struct;
 + bool ret_struct:1;
  
   union {
   gregs32_t r32;
   gregs64_t r64;
   } regs;
 - struct fpregs_t fpregs;
  
 + struct fpregs_t fpregs;
 + bool heterog:1;
 + bool hfa_type:1;

According to the code below, heterog and hfa_type are not part of
_state_.  You don't need to keep their values between calls.  Why are
they in context?

Also, hfa_type is a good name for something that is a type (in ltrace
typically a variable of type enum arg_type or possibly struct
arg_type_info).  A predicate might be called is_hfa_type, or hfa_type_p,
or some such.

 @@ -124,9 +132,36 @@ arch_fetch_arg_init(enum tof type, struct process *proc,

[...]

 + if (context-ret_struct) {
 +#if _CALL_ELF != 2
   context-greg++;
 +#else
 + /* if R3 points to stack, parameters will be in R4 */
 + long pstack_end = ptrace(PTRACE_PEEKTEXT, proc-pid,
 + proc-stack_pointer,0);

There should be a space after comma.

 + if (( (long)context-regs.r64[3]  (long)proc-stack_pointer )
 +  ( (long)context-regs.r64[3]  pstack_end ) ) {
 + context-greg++;
 + context-stack_pointer += 8;
 + }

Don't put spaces around parenthesized arguments.

Also, the first cast should be to arch_addr_t, the second should be
absent altogether.  If you make pstack_end unsigned, I suspect you will
be able to avoid the bottom cast as well.

 @@ -216,18 +258,34 @@ align_small_int(unsigned char *buf, size_t w, size_t sz)

[...]

 +
 +#if _CALL_ELF == 2
 + /* Consume the stack slot corresponding to this arg */
 + if ( (int)(sz + off ) = 8 ) {

Why do you cast to int here?
There should be a period and two spaces before */
There shouldn't be spaces around the parenthesized argument.

 + ctx-greg++;
 + }
 + if ( ctx-hfa_type )
 + ctx-stack_pointer += sz ;
 + else
 + ctx-stack_pointer += 8 ;

There should be no space before a semicolon.  The if has wrong spacing
as well.

 @@ -258,7 +317,10 @@ allocate_float(struct fetch_context *ctx, struct process 
 *proc,
  
   ctx-freg++;
   if (proc-e_machine == EM_PPC64)
 - allocate_gpr(ctx, proc, info, NULL);
 + allocate_gpr(ctx, proc, info, NULL, off);
 +
 + if ( ! ctx-hfa_type )
 + ctx-vgreg++;

Again, no spaces inside parentheses.

 @@ -276,9 +338,140 @@ allocate_float(struct fetch_context *ctx, struct 
 process *proc,
  }
  
  static int
 +allocate_hfa(struct fetch_context *ctx, struct process *proc,
 +   struct arg_type_info *info, struct value *valuep,
 +   enum arg_type hfa_type, size_t hfa_count)
 + {
 +  size_t sz = type_sizeof(proc, info);
 +  if (sz == (size_t)-1)
 +  return -1;
 +
 + ctx-hfa_size += sz;;

No double semicolons please.

 +  unsigned char *buf = value_reserve(valuep, sz);
 +  if (buf == NULL)
 +  return -1;

Up here, you have a tab followed by a space.  That's probably wrong.
It's OK down here:

 +
 + ctx-hfa_type = 1 ;

true, not 1.  Also, no space before semicolon.

 + ctx-heterog = 0 ;
 + if ( hfa_count  8 ) {
 + ctx-heterog = 1 ;
 + }

Again, too much spacing throughout.

 +
 +
 +  struct arg_type_info *hfa_info = type_get_simple(hfa_type);
 +  size_t hfa_sz = type_sizeof(proc, hfa_info);
 +
 +

The following while has again wrong indentation:

 +  while ( hfa_count  0  ctx-freg = 13 ) {
 + int rc;
 +  struct value tmp;
 +
 +  value_init(tmp, proc, NULL, hfa_info, 0);
 +
 + /* Hetereogeneous struct - get value on GPR or stack */

You keep saying hetereogeneous and I keep wondering whether you mean
homogeneous, or whether heterogeneous structs really are something in
context of ELFv2 ABI.  Like, I could imagine mixed float/double structs
(i.e. heterogeneous) getting the sort of optimization that homogeneous
normally do.  But I don't know.  Could you 

Re: [Ltrace-devel] git branch w/ some missing includes

[Petr Machata]

dann frazier da...@debian.org writes:

  I posted some patches yesterday that added some missing includes in the
 new aarch64 code (held for moderation since I wasn't yet sub'd). I've
 posted my git branch here if you'd prefer to merge from there:
   https://github.com/dannf/ltrace

Sorry, I didn't notice your e-mail.  I kinda assumed the unread in my
ltrace folder were my convesation with Thierry about ppcle/ELFv2
backend.

Both patches look fine, I applied them on master.

I haven't had list admin privileges, but I've requested them now, so
hopefully I'll be able to address these issues directly next time.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] ltrace update for ppc64el arch (Petr Machata)

[Petr Machata]

Thanks, this will save me quite a bit of work figuring out how the new
ABI works.

Thierry@vnet thie...@linux.vnet.ibm.com writes:

 diff --git a/ltrace-0.7.3.orig/sysdeps/linux-gnu/ppc/arch.h 
 b/ltrace-0.7.3/sysdeps/linux-gnu/ppc/arch.h
 index fb8768a..edabe2e 100644
 --- a/ltrace-0.7.3.orig/sysdeps/linux-gnu/ppc/arch.h
 +++ b/ltrace-0.7.3/sysdeps/linux-gnu/ppc/arch.h
 @@ -27,6 +27,7 @@
  #define BREAKPOINT_VALUE { 0x7f, 0xe0, 0x00, 0x08 }
  #define BREAKPOINT_LENGTH 4
  #define DECR_PC_AFTER_BREAK 0
 +#define ARCH_ENDIAN_BIG
  
  #define LT_ELFCLASS  ELFCLASS32
  #define LT_ELF_MACHINE   EM_PPC
 @@ -35,6 +36,12 @@
  #define LT_ELFCLASS2 ELFCLASS64
  #define LT_ELF_MACHINE2  EM_PPC64
  #define ARCH_SUPPORTS_OPD
 +#ifdef __LITTLE_ENDIAN__
 +#undef BREAKPOINT_VALUE
 +#define BREAKPOINT_VALUE { 0x08, 0x00, 0xe0, 0x7f }
 +#define ARCH_ENDIAN_LITTLE
 +#undef ARCH_ENDIAN_BIG
 +#endif

Please don't #undef, instead conditionally #define one or the other.

 diff --git a/ltrace.0.7.3-v1/ltrace-elf.c b/ltrace-0.7.3/ltrace-elf.c
 index c571d2a..4b8b0bc 100644
 --- a/ltrace.0.7.3-v1/ltrace-elf.c
 +++ b/ltrace-0.7.3/ltrace-elf.c
 @@ -714,6 +714,10 @@ populate_this_symtab(struct Process *proc, const char 
 *filename,
   continue;
   }
  
 +#if defined(__powerpc64__)  _CALL_ELF == 2
 + naddr += PPC64_LOCAL_ENTRY_OFFSET (sym.st_other);
 +#endif
 +

Where does _CALL_ELF come from?  What is PPC64_LOCAL_ENTRY_OFFSET?  In
any case, this should be folded into arch_translate_address in PPC
backend.

 +#if _CALL_ELF == 2
 +#undef ARCH_SUPPORTS_OPD
 +#endif

Again, either #define or don't #define, but don't #undef.

 +enum homogeneous_type {
 + NOINIT = 0,
 + HETEROGENEOUS,
 + HOMOGENEOUS,
 + HOMOGENEOUS_NESTED_FLOAT,
 +};
 +
 +struct struct_attributes {
 + struct arg_type_info *info;
 + enum arg_type type;
 + size_t nb_elements;
 + enum homogeneous_type homogeneous;
 +};
 +
 +

Why can't you use type_get_hfa_type?  Can we extend it so that it does
what you need?

 + if (ret_info-type == ARGTYPE_STRUCT) {
 + get_struct_attribut(ret_info, proc, ret_size, struct_attr);
 +
 + if (((struct_attr.homogeneous == HOMOGENEOUS_NESTED_FLOAT) 
 +(struct_attr.nb_elements  8)) ||
 +(((struct_attr.homogeneous == HOMOGENEOUS) ||
 +  (struct_attr.homogeneous == HETEROGENEOUS)) 
 + (ret_size  16)))

Please don't leave  and || hanging on the end of a line.  Also indent
properly (what follows after the first  should be two characters
ahead).  Personally I'd write it thus:

+   if ((struct_attr.homogeneous == HOMOGENEOUS_NESTED_FLOAT
+ struct_attr.nb_elements  8)
+   || ((struct_attr.homogeneous == HOMOGENEOUS
+|| struct_attr.homogeneous == HETEROGENEOUS)
+ret_size  16))

There are more instances of this problem, I won't cite them all.

 +struct arg_type_info *
 +arch_type_get_fp_equivalent(struct arg_type_info *info, struct Process *proc)
 +{

What is this good for?

   unsigned char *buf = value_reserve(valuep, slots * width);
   if (buf == NULL)
   return -1;
 - struct arg_type_info *long_info = type_get_simple(ARGTYPE_LONG);
 + struct arg_type_info *long_info = 
 arch_type_get_simple(ARGTYPE_LONG,proc);

I see you undid this change in a follow-up patch.  It would be better if
you could fold the fixes into this one, so that each patch can be
reviewed and tested separately.

 +#if _CALL_ELF != 2
  #define PPC64_PLT_STUB_SIZE 8 //xxx
 +#else
 +#define PPC64_PLT_STUB_SIZE 4 //xxx
 +#endif

Let's drop both xxx's.  Cleary that's what it is at this moment.

  arch_translate_address(struct ltelf *lte,
  arch_addr_t addr, arch_addr_t *ret)
  {
 - if (lte-ehdr.e_machine == EM_PPC64) {
 + if (lte-ehdr.e_machine == EM_PPC64
 + (lte-ehdr.e_flags  3) != 2 ) {

Do these numbers have symbolic names by any chance?

   /* XXX The double cast should be removed when
* arch_addr_t becomes integral type.  */
   GElf_Xword offset
 @@ -433,6 +444,7 @@ int
  arch_elf_init(struct ltelf *lte, struct library *lib)
  {
   if (lte-ehdr.e_machine == EM_PPC64
 +  (lte-ehdr.e_flags  3) != 2

And again here... clearly we need to name this and refer back to it
instead of testing the same thing again and again.  It seems as if the
following is necessary:

diff --git a/sysdeps/linux-gnu/ppc/arch.h b/sysdeps/linux-gnu/ppc/arch.h
index bf9b5dc..664a670 100644
--- a/sysdeps/linux-gnu/ppc/arch.h
+++ b/sysdeps/linux-gnu/ppc/arch.h
@@ -56,7 +56,8 @@ struct arch_ltelf_data {
Elf_Data *opd_data;
GElf_Addr opd_base;
GElf_Xword opd_size;
-   int secure_plt;
+   bool secure_plt : 1;
+   bool elfv2_abi : 1;
 
Elf_Data *reladyn;
size_t reladyn_count;
@@ -65,7 

Re: [Ltrace-devel] Error in last patch for ltrace ABIv2

[Petr Machata]

Thierry@vnet thie...@linux.vnet.ibm.com writes:

 1) The change to config.guess and libtool.m4 are supposed to be done, if
 not let me know.

Those look fine to me.  You might want to post those separately, but
depends on you, I don't mind having them in one bunch with the rest of
ELFv2 enablement.

 2) There was an error in making the patch, here is the correction
 Sorry about that

Please fold this to a single patch.

Thank you,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Git errors this week-end

[Petr Machata]

Thierry@vnet thie...@linux.vnet.ibm.com writes:

 git clone git://git.debian.org/git/collab-maint/ltrace.git
 Cloning into 'ltrace'...
 fatal: protocol error: bad line length character:

It works for me now.  Must have been a blip.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Current dev. tree tes run

[Petr Machata]

Thierry@vnet thie...@linux.vnet.ibm.com writes:

 I worked this morning with the current dev. tree on ppc64 and i386 and
 both are failing the ./ltrace.main/system_calls.exp with the error :
   FAIL: ^(new|f)?stat(64)?$ was recorded 2 times, expected == 1
 Looking at the output we find 2 breakpoints
   __xstat
   fstat
 which could explain that the line 136 : { {^(new|f)?stat(64)?$} == 1 }
 would have to be changed to : { {^(new|f)?stat(64)?$} == 2 }

Yeah, that test is fragile.  Each arch has a different implementation of
same system call wrappers, so this ends up fairly noisy and not very
useful.  Please replace the == 1 with = 1.

Thank you,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] Work on bug #135985 - ltrace could catch library-to-library calls

[Petr Machata]

Jevin Sweval jevinswe...@gmail.com writes:

 This is a feature that I would really like to see! I'm not sure where
 to start working though.

This should generally work in ltrace 0.7.0 or later.  Use ltrace -e@ to
tell ltrace that you want to trace calls from all libraries (the empty
string before the @ means all symbols, the empty string after @ means
all libraries.  Check out man for more elaborate syntax).  -n sets
indentation so that it's clearer what's happening:

$ ltrace -e@ -n1 echo
echo-__libc_start_main([ echo ] unfinished ...
 echo-getenv(POSIXLY_CORRECT)
 = nil
 echo-strrchr(echo, '/') 
 = nil
 echo-setlocale(LC_ALL,  unfinished ...
  libc.so.6-malloc(5)  
 = 0x187a010
  libc.so.6-free(0x187a010)
 = void
  [... etc ...]

Note that the method of tracing that ltrace uses has a very high
overhead.  Using e.g. python in this regime is an interesting
contemplative exercise--it takes minutes before you see the prompt and
then every character you write produces a sputter of log messages.

 I don't understand Juan's comment [0] about how libc6 libraries don't
 use the executable's PLT (do they use their own?). I would greatly
 appreciate any pointers!

Each library has its own set of PLT slots through which it calls to
other libraries.  Formerly ltrace only cared about the PLT slots in the
main executable.  The missing functionality was added in 0.7.0.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

[Ltrace-devel] Some MIPS backend work

[Petr Machata]

This e-mail went out over a week ago.  I noticed just now that I forgot
to CC: the ltrace devel list, so I'm resending it.  I haven't had a
chance to work on the MIPS backend since I wrote this.

--

Hi there,

I had a chance to look into MIPS tracing on Sunday.  For the PLT-ful
case we can just reuse what core does, so that's not a problem.

In PLT-disabled binaries, the calls are done through addresses fetched
from GOT.

00400660 main:
  [...]
  400670:   3c1c0042lui gp,0x42
  400674:   279c8900addiu   gp,gp,-30464 -- gp = some global 
pointer?
  [...]
  40068c:   8f828034lw  v0,-32716(gp) -- address from GOT
  400690:   00200825moveat,at
  400694:   0040c821movet9,v0
  400698:   0320f809jalrt9-- jump to it
  40069c:   00200825moveat,at

GOT initially contains addresses that point to .MIPS.stubs section,
where the jump leads:

  400820:   8f998010lw  t9,-32752(gp) -- t9 = GOT[0]
  400824:   03e07821movet7,ra -- t7 = caller's return 
addr
  400828:   0320f809jalrt9
  40082c:   24180009li  t8,9  -- t8 = .dynsym index

After the first call, GOT entry is resolved and contains address of the
resolved symbol.  So .MIPS.stubs is avoided except on first call, and
possibly even then, if the binary has been prelinked (but I haven't had
a chance to check that, my qemu image isn't prelinked).

So we have to do the same relatively complicated thing that PPC backend
does: the first time a stub breakpoint is hit, we single-step through
dynamic linker, looking for when the corresponding GOT entry is updated.
When it is, we rewrite it back, but remember the resolved address.  When
this stub breakpoint is hit the second time, we just move PC over to
that resolved address that we remembered (and update $t9 as well).

This is now partly implemented on pmachata/mips.  It doesn't handle
prelinked binaries yet, and I didn't try to run the test suite yet
eihter.  But it's a good start, and already it could give reasonable
traces for ls, pwd, and a couple test binaries.

The original code placed the breakpoint at the resolved address, and
knew to update it after the function returns.  Unfortunately that
creates a window between when the call is resolved, and when it returns,
where any calls (like those from other threads, or recursive calls done
through PLT) would be missed.  That's why I ported the fairly
complicated PPC approach.

Originally I wanted to leave out an optimization that PPC backend does,
whereby you remember the address of the instruction which resolved a GOT
entry, and put a breakpoint there.  Next time, instead of
single-stepping through dynamic linker, you just let the traced process
hit this breakpoint.  Now MIPS doesn't allow PTRACE_SINGLESTEP, and we
must do it in software, so single-stepping is super expensive.  In my
qemu, the first instance of each library call took like half a second to
resolve, and I'm afraid that on the often low-power MIPS boxes, this
could be similarly bad.  So eventually I put the optimization in.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] missing library traces

[Petr Machata]

Oliver Metz oli...@freetz.org writes:

 Is this a ltrace bug? What information can I provide to help to fix
 it? Or does our toolchain misbehave? I read some mails from Sedat in
 the archives about the PLT topic but nothing the helped me to solve
 this issue. You can find some more information in our Trac ticket
 (http://freetz.org/ticket/2326).

It might be a missing feature.  ltrace generally relies on existence of
PLT slots, through which all call to dynamic libraries are made.  We put
breakpoints to these slots, so as to catch these calls.  If there are no
PLT slots, we can't trace library calls.

In practice, on PowerPC in particular, PLT slots themselves are not
really useful for these breakpoints and considerable hackery is
necessary to achieve the effect as if we were tracing PLT calls.
Perahaps MIPS backend could do something unusual to trace the -mno-plt
case as well.  It all depends on how tricky it would be to support, and
whether it's the common case.  From a couple Google hits, I gather -mplt
really is the usual case, but maybe that's different for embedded
systems.

As they say, we accept patches ;)

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] missing library traces

[Petr Machata]

Eugene Rudoy g...@freetz.org writes:

 as to compiling the master: I had to apply the two patches attached to this
 mail in order to compile it (I simply removed that
 lte-relplt_count-line).

Hmm, the _GNU_SOURCE ones probably make sense (see the comment about
uclibc above one of them).

 As to fixing the issue, in particular It shouldn't be hard for MIPS'
 arch_elf_init to add any special entries: sounds interesting ;-) but
 it's something completely new to us (Oliver  me), we just cross-compile
 ltrace and make it available to the users of freetz-project.

 Would that be possible for you to take a deeper look into the issue? Is
 there something we can do to support you? Testing, hardware (probably
 remote access only)?

In fact I'm installing Debian Wheezy inside Qemu as I write this.
I can't promise anything (this would have to go from my personal time
budget, which is already fairly strained), but I should be able to at
least take a look.

PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel

Re: [Ltrace-devel] [PATCH] Add support for using elfutils as unwinder.

[Petr Machata]

Mark Wielaard m...@redhat.com writes:

 In the ltrace case it might just be enough to print  [...] in case of
 any error to indicate we don't know whether or not there should be more
 frames (beyond the -w NR limit).

 I still wonder what your opinion is though.  It seems as if
 dwfl_thread_getframes should return a different error number on line
 436, but that ship has sailed.

 Yeah, but it is hard to know which one. Since there are various things
 that can cause us getting in that error state. The stack could be
 corrupted, missing .eh_frame, corrupted/bad CFI data, etc. And on a
 modern up to date system it really should return success.

I meant returning e.g. -2 instead of -1 as a result, just to indicate
that it unwound something, but had to give up.  Your solution with
[...]  seems reasonable.

Thanks,
PM

___
Ltrace-devel mailing list
Ltrace-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel