This is an archive of the discontinued LLVM Phabricator instance.

[Headers] Remove a space in NULL define
ClosedPublic

Authored by aheejin on Aug 31 2023, 2:43 PM.

Details

Summary

There was no space in ((void *)0) before D158709. This can cause
downstream warnings in case other libraries define NULL as
((void*)0), which is the case for musl. (see NULL definition in
https://git.musl-libc.org/cgit/musl/tree/include/stdio.h)

When a macro is redefined, if the content is the same it is fine, but if
it is different even in terms of a single space, clang warns:

../musl/include/stdio.h:37:9: error: 'NULL' macro redefined [-Werror,-Wmacro-redefined]
   37 | #define NULL ((void*)0)
      |         ^

The old code didn't have the space and it had been fine for many years,
so I think there's no risk in removing it. The linter seems to prefer
the space in there, but I think it has a risk of causing warnings or
even errors for downstream users.

Diff Detail

Event Timeline

aheejin created this revision.Aug 31 2023, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2023, 2:43 PM
Herald added a subscriber: wingo. · View Herald Transcript
aheejin requested review of this revision.Aug 31 2023, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2023, 2:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aheejin edited the summary of this revision. (Show Details)Aug 31 2023, 2:44 PM
aheejin edited the summary of this revision. (Show Details)Aug 31 2023, 2:48 PM
iana accepted this revision.Aug 31 2023, 5:07 PM

Can you add a comment that says the spacing is important please? Although why the heck is musl declaring NULL in stdio.h?

This revision is now accepted and ready to land.Aug 31 2023, 5:07 PM
aheejin updated this revision to Diff 555201.Aug 31 2023, 5:19 PM

Add comment

Done. Btw musl defines NULL in several places, not only stdio.h. It defines NULL in local.h, stddef.h, stdio.h, stdlib.h, string.h, time.h, unistd.h, and wchar.h. Not sure why.

This revision was landed with ongoing or failed builds.Aug 31 2023, 5:23 PM
This revision was automatically updated to reflect the committed changes.
aaron.ballman added inline comments.
clang/lib/Headers/__stddef_null.h
18–21

My concern with this comment is that it implies we can never change our definition of NULL -- as the implementation, this is ours to define and if other C standard libraries (also part of the implementation!) are redefining the macro, IMO, they need to solve this themselves with either #ifdef or #undef as needed instead of expecting the definitions to stay in lock-step including whitespace. I don't expect us to change the definition of NULL any time soon, but this feels like a bad precedent to set. I'm not opposed (this is solving a real problem), but I'm wondering if musl would consider guarding their macro definitions so we don't have this fragility forever? We need to keep it for now so existing musl uses don't have this problem, but I don't really want to make the guarantees this comment is claiming.

dalias added a subscriber: dalias.Sep 1 2023, 5:07 AM

I don't understand the motivation of trying to match musl's definition here. musl explicitly does not support using a compiler-provided stddef.h or other standard headers. If it's getting included, this is a symption of an include order problem that needs to be fixed, and getting an error telling you that is preferable.

I don't understand the motivation of trying to match musl's definition here. musl explicitly does not support using a compiler-provided stddef.h or other standard headers. If it's getting included, this is a symption of an include order problem that needs to be fixed, and getting an error telling you that is preferable.

Oh! Thank you for that information, that's really good to know! Our stddef.h is one of the few headers we provide that doesn't do an include_next to get to the system library header if one is available; I wonder if that's a contributing factor?

aheejin added a comment.EditedSep 1 2023, 8:32 AM

I also think this is better solved downstream, i.e., in musl. We not a musl developer but a user, but we can consider contributing a patch there. I'd like to remove the whitespace for now because it is currently breaking us and it had been that way for a long time anyway, but I don't mind removing the comments, given that it sounds like we cannot change this forever.

dalias added a comment.Sep 1 2023, 9:07 AM

Please report what you're actually trying to do that's breaking rather than sending patches to align definitions that are not intended to be aligned.

Please report what you're actually trying to do that's breaking rather than sending patches to align definitions that are not intended to be aligned.

Basically, we tried to use stddef.h (from clang) and stdio.h (from musl) at the same time. Didn't know it wasn't supposed to be supported in the first place.
In more detail, our musl has some emscripten-specific modifications like this, and this lead to include stddef.h. But this problem can happen whenever we try to use the two headers together.

I get removing the whitespace here is probably not the right long-term solution. What do you think we (emscripten) or clang should do?

I also think it is a good idea to remove the comment here because we don't want to say this is a long-term solution. But I hope we keep the code as whitespace-free for now, given that at least it can be a band-aid to users like us, and it didn't have the whitespace for years before D158709 anyway.

Are we keeping the comment?

I uploaded D159383, which removes the comments.