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: Joseph Myers <joseph at codesourcery dot com>
- To: Yury Norov <ynorov at caviumnetworks 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>, <adhemerval dot zanella at linaro dot org>, Yury Norov <yury dot norov at gmail dot com>
- Date: Fri, 1 Jul 2016 12:05:32 +0000
- 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 Fri, 1 Jul 2016, 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 */
No. Defining to 0 or 1 is the right thing to do - but you need to
identify *all* relevant bits/typesizes.h headers (if that's the bits/
header you use for this) and update *all* of them to define the macro to
the appropriate value.
In general, if a new port needs something new, you must think first in
terms of architecture-independent design for how to handle the new issue
in the logically best and cleanest way, and then make any necessary
changes / refactoring for all existing ports. Not just hack up whatever
seems most convenient for your port without thinking about the logically
correct design to cover all existing and expected future ports.
> > > + 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.
Still wrong. The "if" line should be indented by two columns, the "{" and
"}" by four columns and the conv_timespec calls by six columns.
> > > +# define conv_timespec(ts, _ts) \
> >
> > Again, not valid in installed headers.
>
> What the proper place you suggest? New timer-related private header?
If something is only needed in the implementation, it should go in an
internal header, not installed.
If something is needed in the installed headers but is not part of the
public, documented (generally system-independent) API, it must have a name
in the implementation namespace.
No, I'm not saying "macro X should go in header Y". I'm giving the
principles that you, understanding the issues you are trying to address,
can use to determine what is correct. It's your responsibility as a patch
submitter to apply those principles to the port you are implementing, and
to write up your patch submissions in sufficient depth to make clear to
the reviewers how you have considered and addressed those principles.
--
Joseph S. Myers
joseph@codesourcery.com