Hi Dmitry,
On 16/05/14 17:02, Dmitry V. Levin wrote:
> On Fri, May 16, 2014 at 12:53:29PM +0100, James Hogan wrote:
>> On 14/05/14 15:17, Dmitry V. Levin wrote:
> [...]
>>> I have two questions wrt this patch:
>>>
>>> 1. Removing the conditional would result with print_rlimit32 defined for
>>> all architectures, including pure 64bit ones. I suppose we need a check
>>> like this anyway:
>>>
>>> #if PERSONALITY0_WORDSIZE == 4 || \
>>> (SUPPORTED_PERSONALITIES > 1 && PERSONALITY1_WORDSIZE == 4) || \
>>> (SUPPORTED_PERSONALITIES > 2 && PERSONALITY2_WORDSIZE == 4)
>>
>> There don't appear to be any arches supported (according to defs.h) with
>>> 2 personalities with all wordsizes the same.
>>
>> Where there are 2 personalities with the same wordsize the definition of
>> current_wordsize takes this into account and defines it as
>> PERSONALITY0_WORDSIZE.
>
> Yes, so the check could be simplified to
> #if !defined(current_wordsize) || current_wordsize == 4
Unfortunately this confuses preprocessor when using this definition:
# define PERSONALITY0_WORDSIZE (int)(sizeof(long))
Maybe with this extra hunk?
diff --git a/defs.h b/defs.h
index 074c8f0..4e06a92 100644
--- a/defs.h
+++ b/defs.h
@@ -370,7 +370,7 @@ struct arm_pt_regs {
# define DEFAULT_PERSONALITY 0
#endif
#ifndef PERSONALITY0_WORDSIZE
-# define PERSONALITY0_WORDSIZE (int)(sizeof(long))
+# define PERSONALITY0_WORDSIZE SIZEOF_LONG
#endif
#if defined(I386) || defined(X86_64)
>> So this would seem to allow the compiler to optimise out whichever
>> print_rlimit{32,64} is unused, and a quick look at the x86_64 assembly
>> output (gcc 4.7.3 -O2) with a hacked current_wordsize in resource.c
>> confirms that.
>
> Yes, and some day the compiler will also optimize out decode_rlimit
> because it ends up the same as decode_rlimit64.
> I'd rather not wait when it happens and add the check.
Ah yes, I see the reasoning behind it now :)
>
>> There is of course the "!addr" and "!verbose(tcp || (exiting(tcp) &&
>> syserror(tcp))" conditions which seem to be skipped for the pure 64-bit
>> case at the moment which I'm not sure was intentional anyway?
>
> Hopefully, they are not skipped. I admit these function names
> (sprint_rlim64, print_rlimit64, and decode_rlimit64) are somewhat
> confusing. The last of them does all these top level checks.
Yep, my mistake. I was confusing the functions and reading print_rlimit64.
>
>>> 2. I'm not sure about architectures where kernel wordsize is 8 but user
>>
>>> wordsize is 4 (like x32 and mips n32); what's the correct rlim_t for these
>>> architectures?
>>
>> Hmm, that's a good point.
>>
>> MIPS n32 appears to use the default __kernel_ulong_t define of unsigned
>> long (32bit) so it would work fine I think (although sadly it appears
>> that strace doesn't support personalities on MIPS yet).
>>
>> x32 appears to define __kernel_ulong_t as unsigned long long (64bit), so
>> strace handling of {get,set}rlimit would already appear to be broken for
>> that (this patch doesn't make that any worse).
>
> Thanks. I suppose this patch fixes all these rlim_t issues:
>
> --- a/configure.ac
> +++ b/configure.ac
> @@ -309,7 +309,6 @@ AC_CACHE_CHECK([for BLKGETSIZE64],
> [ac_cv_have_blkgetsize64],
> AC_CHECK_SIZEOF([long])
> AC_CHECK_SIZEOF([long long])
> AC_CHECK_SIZEOF([off_t],,[#include <sys/types.h>])
> -AC_CHECK_SIZEOF([rlim_t],,[#include <sys/resource.h>])
>
> AC_CACHE_CHECK([for SA_RESTORER], [st_cv_sa_restorer],
> [st_cv_sa_restorer="$(echo SA_RESTORER |
> --- a/resource.c
> +++ b/resource.c
> @@ -88,10 +88,6 @@ static const struct xlat resources[] = {
> XLAT_END
> };
>
> -#if !(SIZEOF_RLIM_T == 4 || SIZEOF_RLIM_T == 8)
> -# error "Unsupported SIZEOF_RLIM_T value"
> -#endif
> -
> static const char *
> sprint_rlim64(uint64_t lim)
> {
> @@ -135,7 +131,7 @@ decode_rlimit64(struct tcb *tcp, unsigned long addr)
> print_rlimit64(tcp, addr);
> }
>
> -#if SIZEOF_RLIM_T == 4 || SUPPORTED_PERSONALITIES > 1
> +#if !defined(current_wordsize) || current_wordsize == 4
>
> static const char *
> sprint_rlim32(uint32_t lim)
> @@ -176,22 +172,22 @@ decode_rlimit(struct tcb *tcp, unsigned long addr)
> else if (!verbose(tcp) || (exiting(tcp) && syserror(tcp)))
> tprintf("%#lx", addr);
> else {
> -# if SIZEOF_RLIM_T == 4
> - print_rlimit32(tcp, addr);
> +# ifdef X32
Does this need to take X86_64 into account too, since I presume
personality 1 is X32 too?
Thanks
James
> + if (current_personality == 1)
> # else
> if (current_wordsize == 4)
> +# endif
> print_rlimit32(tcp, addr);
> else
> print_rlimit64(tcp, addr);
> -# endif
> }
> }
>
> -#else /* SIZEOF_RLIM_T == 8 && SUPPORTED_PERSONALITIES == 1 */
> +#else /* defined(current_wordsize) && current_wordsize != 4 */
>
> # define decode_rlimit decode_rlimit64
>
> -#endif /* SIZEOF_RLIM_T == 4 || SUPPORTED_PERSONALITIES > 1 */
> +#endif
>
> int
> sys_getrlimit(struct tcb *tcp)
>
>
------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
