gcc headers
ClosedPublic
Actions

Authored by vexeduxr on Mon, Feb 23, 5:37 AM.

Details

Reviewers

brooks
kib
imp

Commits

rG0c075db78a98: tools/build/stddef.h: fix stock clang/gcc headers

Summary

Both clang and gcc's stddef.h are designed to be included multiple
times with different combinations of __need_* macros defined (e.g
__need_size_t). Remove the #pragma once to accommodate this,
ptraddr_t is guarded by _PTRADDR_T_DECLARED anyways.

Also use __SIZE_TYPE__ instead of size_t since it's not guaranteed to be
defined.

Edit: put commit message in a code block to avoid markdown shenanigans

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 70899
Build 67782: arc lint + arc unit

Event Timeline

vexeduxr created this revision.Mon, Feb 23, 5:37 AM

Herald added a subscriber: imp. · View Herald TranscriptMon, Feb 23, 5:37 AM

Harbormaster completed remote builds in B70899: Diff 172482.Mon, Feb 23, 5:37 AM

vexeduxr requested review of this revision.Mon, Feb 23, 5:37 AM

vexeduxr edited the summary of this revision. (Show Details)Mon, Feb 23, 12:20 PM

vexeduxr edited the summary of this revision. (Show Details)

Also use SIZE_TYPE instead of size_t since it's not guaranteed to be defined.

Where this happens? size_t in stddef.h is required by all C standards AFAIR.

It happens if anything other than __need_size_t is defined. Some headers include stddef.h but don't want the whole thing, so they define __need_*. The header can also be included multiple times with different __need_* macros defined.
This is all internal of course, simply including stddef.h with nothing defined gives you the whole thing.

A comment from clang's stddef.h:

This header is designed to be included multiple times. If any of the __need_
macros are defined, then only that subset of interfaces are provided. This
can be useful for POSIX headers that need to not expose all of stddef.h, but
need to use some of its interfaces. Otherwise this header provides all of
the expected interfaces.

kib accepted this revision.Mon, Feb 23, 7:38 PM

This revision is now accepted and ready to land.Mon, Feb 23, 7:38 PM

I don't think the commit text is specific enough about the cause. You should point to a specific combination that fails, not claim its is was macOS only (I'm fairly sure I added this due to testing cross build on ubuntu).

tools/build/stddef.h
39	I think this should remain or be converted to ifdef guards.

In D55453#1269284, @brooks wrote:

I don't think the commit text is specific enough about the cause. You should point to a specific combination that fails, not claim its is was macOS only (I'm fairly sure I added this due to testing cross build on ubuntu).

The build currently fails on ubuntu, at least on our Github runners [1].
Anything that uses the stock clang or gcc headers would fail AFAICT. I'm not familiar with toolchains on macOS, but it seems like it uses different headers. I'll make the commit message a bit clearer.

[1] https://github.com/freebsd/freebsd-src/actions/runs/22190520083

tools/build/stddef.h
39	That would break headers that include stddef.h multiple times. clang's headers do this a lot internally with different `__need_*` combinations.

brooks accepted this revision.Tue, Feb 24, 11:07 AM

brooks added inline comments.

tools/build/stddef.h
39	The commit message should explain this.

vexeduxr retitled this revision from tools/build/stddef.h: fix non-macos build to tools/build/stddef.h: fix stock clang/gcc headers.Tue, Feb 24, 11:16 AM

vexeduxr edited the summary of this revision. (Show Details)

The new commit message is much better. Thanks

imp accepted this revision.Tue, Feb 24, 2:49 PM

I also wanted to suggest that the reasons should be explained not only in the commit message, but also in the comment in the file itself. I hesitated, since this is tools/build and of limited scope.

But now, since the include guard(s) are removed, which I believe must be explained in the file itself, otherwise somebody would be tempted to re-add them, it is also a good reason to add explanation why size_t is not used directly.

Add comments explaining the missing header guard and the use of SIZE_TYPE

This revision now requires review to proceed.Tue, Feb 24, 4:44 PM

Harbormaster completed remote builds in B70962: Diff 172612.Tue, Feb 24, 4:44 PM

Thanks