This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC PATCH] [AARCH64] ILP32: support stat syscall family
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Yury Norov <ynorov at caviumnetworks dot com>, Joseph Myers <joseph at codesourcery dot com>
- Cc: libc-alpha at sourceware dot org, arnd at arndb dot de, catalin dot marinas at arm dot com, davem at davemloft dot net, szabolcs dot nagy at arm dot com, maxim dot kuvyrkov at linaro dot org, pinskia at gmail dot com, bamvor dot zhangjian at huawei dot com, schwab at suse dot de, fweimer at redhat dot com, Prasun dot Kapoor at cavium dot com, Yury Norov <yury dot norov at gmail dot com>
- Date: Fri, 1 Jul 2016 09:12:47 -0300
- Subject: Re: [RFC PATCH] [AARCH64] ILP32: support stat syscall family
- Authentication-results: sourceware.org; auth=none
- References: <1467291615-30356-1-git-send-email-ynorov at caviumnetworks dot com> <alpine dot DEB dot 2 dot 20 dot 1606301347050 dot 27126 at digraph dot polyomino dot org dot uk> <20160701090301 dot GA14016 at yury-N73SV>
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
>