Re: [PATCH] aix: handle 64bit inodes for include directories

[Jeff Law <jeffreyalaw@gmail.com>]

On 6/28/2021 1:16 AM, CHIGOT, CLEMENT wrote:
>> On 6/23/2021 12:53 AM, CHIGOT, CLEMENT via Gcc-patches wrote:
>>> Hi David,
>>>
>>> Did you have a chance to take look at this patch ?
>>>
>>> Thanks,
>>> Clément
>>>
>>>
>>>> +DavidMalcolm
>>>>
>>>> Can you review this patch when you have a moment?
>>>>
>>>> Thanks, David
>>>>
>>>> On Mon, May 17, 2021 at 3:05 PM David Edelsohn <dje.gcc@gmail.com> wrote:
>>>>> The aix.h change is okay with me, but you need to get approval for the
>>>>> incpath.c and cpplib.h parts of the patch from the appropriate
>>>>> maintainers.
>>>>>
>>>>> Thanks, David
>>>>>
>>>>> On Mon, May 17, 2021 at 7:44 AM CHIGOT, CLEMENT <clement.chigot@atos.net> wrote:
>>>>>> On AIX, stat will store inodes in 32bit even when using LARGE_FILES.
>>>>>> If the inode is larger, it will return -1 in st_ino.
>>>>>> Thus, in incpath.c when comparing include directories, if several
>>>>>> of them have 64bit inodes, they will be considered as duplicated.
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>> 2021-05-06  Clément Chigot  <clement.chigot@atos.net>
>>>>>>
>>>>>>            * configure.ac: Check sizeof ino_t and dev_t.
>>>>>>            * config.in: Regenerate.
>>>>>>            * configure: Regenerate.
>>>>>>            * config/rs6000/aix.h (HOST_STAT_FOR_64BIT_INODES): New define.
>>>>>>            * incpath.c (HOST_STAT_FOR_64BIT_INODES): New define.
>>>>>>            (remove_duplicates): Use it.
>>>>>>
>>>>>> libcpp/ChangeLog:
>>>>>> 2021-05-06  Clément Chigot  <clement.chigot@atos.net>
>>>>>>
>>>>>>            * configure.ac: Check sizeof ino_t and dev_t.
>>>>>>            * config.in: Regenerate.
>>>>>>            * configure: Regenerate.
>>>>>>            * include/cpplib.h (INO_T_CPP): Change for AIX.
>>>>>>            (DEV_T_CPP): New macro.
>>>>>>            (struct cpp_dir): Use it.
>> So my worry here is this is really a host property -- ie, this is
>> behavior of where GCC runs, not the target for which GCC is generating code.
>>
>> That implies that the change in aix.h is wrong.  aix.h is for the
>> target, not the host -- you don't want to define something like
>> HOST_STAT_FOR_64BIT_INODES there.
>>
>> You'd want to be triggering this behavior via a host fragment, x-aix, or
>> better yet via an autoconf test.
> Indeed, would this version be better ? I'm not sure about the configure test.
> But as we are retrieving the size of dev_t and ino_t just above, I'm assuming
> that the one being used in stat directly. At least, that's the case on AIX, and
> this test is only made for AIX.
It's a clear improvement.  It's still checking for the aix target though:

+# Select the right stat being able to handle 64bit inodes, if needed.
+if test "$enable_largefile" != no; then
+  case "$target" in
+    *-*-aix*)
+      if test "$ac_cv_sizeof_ino_t" == "4" -a "$ac_cv_sizeof_dev_t" == 
4; then
+
+$as_echo "#define HOST_STAT_FOR_64BIT_INODES stat64x" >>confdefs.h
+
+      fi;;
+  esac
+fi

Again, we're dealing with a host property.  You might be able to just 
change $target above to $host.  Hmm, that makes me wonder about canadian 
crosses where host != build.    We may need to do this for both the aix 
host and aix build.

Sorry about the delay,
jeff