[RFC PATCH] [AARCH64] ILP32: support stat syscall family

[Adhemerval Zanella]

On 01/07/2016 06:03, Yury Norov wrote:
> On Thu, Jun 30, 2016 at 01:51:22PM +0000, Joseph Myers wrote:
>> On Thu, 30 Jun 2016, Yury Norov wrote:
>>
>>>  - __32_BIT_ABI_SUPPORTS_64_BIT_TIME_T macro introduced to indicate that 32-bit
>>>    ABI has struct __timespec (and maybe __timeval in future) that is compatible
>>>    to 64-bit _timespec;
>>
>> You can't add a macro to one architecture like that then use it in generic 
>> code.  You have to define it in *every* architecture's bits/typesizes.h 
>> before it can be used in generic code.
> 
> Is it OK to do like this then?
> #define __32_BIT_ABI_SUPPORTS_64_BIT_TIME_T /* w/o 1 or 0 */
> 
> Where's a proper place for it? In features.h maybe?

I would say kernel-features.h since you are touching Linux specific code.
Also I would make the sysdeps/unix/sysv/linux/* implementation use the
default and expected case and add a new flag only in the non-default case
(so you won't need to check all current ports to see they fit or not).

> 
>>> +      return INLINE_SYSCALL (fstatfs64, 2, fd, buf);
>>
>> Bad indentation.  Likewise elsewhere in this patch.
>>
>>> +++ b/sysdeps/unix/sysv/linux/aarch64/ilp32/statfs64.c
>>> @@ -0,0 +1,37 @@
>>> +/* Copyright (C) 2011-2015 Free Software Foundation, Inc.
>>
>> Still missing descriptive first line and 2016 in copyright dates.
>>
>>> +   Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011.
>>
>> There should be no "Contributed by" lines in new files.
>>
>>> +  return result;
>>> +#endif
>>> +#if __32_BIT_ABI_SUPPORTS_64_BIT_TIME_T
>>> +      if (!result)
>>> +        {
>>> +	   conv_timespec(&buf->st_atim, &buf->__st_atim);
>>> +	   conv_timespec(&buf->st_mtim, &buf->__st_mtim);
>>> +	   conv_timespec(&buf->st_ctim, &buf->__st_ctim);
>>> +        }
>>>  #endif
>>
>> Indentation way off.
> 
> It looks ugly only in email. If you apply patch, it will be better.

The problem is patch is using whitespace where it should a tab.

> 
>>
>>> +       conv_timespec(&st->st_atim, &st->__st_atim);
>>> +       conv_timespec(&st->st_mtim, &st->__st_mtim);
>>> +       conv_timespec(&st->st_ctim, &st->__st_ctim);
>>
>> Missing space before '('.
> 
> This rule is scary, for sure. I cannot autoreplace all such places as
> it doesn't work for definitions, and I cannot drop whitespaces everywhere
> because it breaks rules. So I have to pay attention to it again and
> again. Script you pointed me doesn't understand difference between
> definition and usage of macro. I spent 2 hours fixing the fixes I made
> following the report of that script.
> 
> Could someone give me clear understanding, how exactly this rule
> improves readability and helps finding bugs. I write it because this
> rule makes real troubles to me, and helps with nothing.

Most of current code guidelines are described at [1].  One tool you can use
with some care is 'indent' [2], however I see this break some code (specially
with inline assembly or some deep macro usage).

[1] https://sourceware.org/glibc/wiki/Style_and_Conventions
[2] https://www.gnu.org/software/indent/manual/indent.html

> 
>>> +    DECLARE_TIMESPEC (st_atim);		/* Time of last access.  */
>>> +    DECLARE_TIMESPEC (st_mtim);		/* Time of last modification.  */
>>> +    DECLARE_TIMESPEC (st_ctim);		/* Time of last status change.  */
>>
>> DECLARE_TIMESPEC is in the user's namespace, not valid in installed 
>> headers.
>>
>>> +#  define conv_timespec(ts, _ts)		\
>>
>> Again, not valid in installed headers.
> 
> 
> What the proper place you suggest? New timer-related private header?
> 
> Yury
>