Re: Problems with 592fc5b349

From: Alejandro R. Sedeño <asedeno@mit.edu>
Date: Jun 1, 2023, 15:20 UTC
Message-ID: <20230601152025.116126-1-asedeno@mit.edu>
In-Reply-To: <CABPp-BHKR5GP2NUFWDMSw-Pnra+yGP0kYAiwu-iWgtu66p-1RQ@mail.gmail.com>

On Thu, June 1, 2023 at 10:33AM Elijah Newren <newren@gmail.com> wrote:

Show 8 quoted lines

> Oh, interesting; none of our platform testing caught this.  After a
> little digging, I'm guessing you're on cygwin < 1.7?  However, I'm
> still surprised you noticed, on any platform.  The only use of the
> DT_* defines in cache.h is in the inline function ce_to_dtype().  The
> only places ce_to_dtype() is used are in (1) unpack-trees.c (which
> includes both cache.h and dir.h) and (2) builtin/ls-files.c (which
> also includes both cache.h and dir.h).  So, as far as I can tell, this
> can't cause compilation issues anywhere.  How did you find this?

I build on an ancient Solaris (5.10), for reasons. One day I'll give up on it, but today is not that day.

> In commits in follow-on series, I moved this inline function to a new
> header, read-cache.h.  name-cache.c does not end up including that
> header, so we could add a #include "dir.h" directive to read-cache.h.

> An alternative fix, if you need something for v2.41.0 (am I guessing
> correctly that you tried out v2.41.0 right after it's release and
> that's when you found this?), would be to move the DT_ defines from
> dir.h to statinfo.h (a header included by both dir.h and cache.h).

Yeah, I built v2.41.0 this morning and saw that my sun4x_510 build failed with DT_REG not defined in cache.h while building add-interactive.c I tried the patch I described earlier (add dir.h to cache.h) and ran into the duplicate `struct dir_entry` in name-hash.c. I'm testing a patch where I move DT_ definitions into a new dtype.h, and include it where needed, but statinfo.h seems resonable.

Show 5 quoted lines

> ... Or
> perhaps another fix is to stop having two things in the codebase named
> "struct dir_entry", since it's bound to cause confusion for humans if
> not also be a lurking timebomb for some future code file that needs
> access to both.

Agreed, though I did not want to pull on that particular thread for fear of what else might unravel.

Show 6 quoted lines

> ... But I still don't understand why any suggestions are
> needed for an immediate fix, since all users of ce_to_dtype() should
> have the necessary headers.  Is there an issue where "inline" is
> ignored, and this function is being defined & compiled for every file
> that includes cache.h, and then the linker removes the duplicates or
> something?

I could believe that gcc 3.4.3 (again, ancient), is not being as clever as newer compilers here.

-Alejandro