This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Wipe some more macros that do not belong in C++ forwarding headers
ClosedPublic

Authored by ldionne on Jan 12 2021, 5:28 PM.

Details

Summary

Submitting upstream from OpenBSD tree:

Wipe some more macros that do not belong in C++ forwarding headers.

Diff Detail

Event Timeline

brad created this revision.Jan 12 2021, 5:28 PM
brad requested review of this revision.Jan 12 2021, 5:28 PM
ldionne requested changes to this revision.Jan 13 2021, 7:15 AM

I did some research and found the following commits (which added these #undefs):

commit c904ad45187326cf47abff0819ab57b971c7c78a
Author: Howard Hinnant <hhinnant@apple.com>
Date:   Thu Jul 26 20:01:13 2012 +0000

    Patch by Andrew C. Morrow:  shims to work around macroized getc and putc on linux.  On my eglibc 2.13 based Debian system 'getc' is a macro defined in
    /usr/include/stdio.h. This decision to make it a macro doesn't seem to
    be guarded by any feature test macro as far as I can see.
    
    llvm-svn: 160799

commit cba3e4ca21992692491af652fa4e84798ef09990
Author: Jonathan Roelofs <jonathan@codesourcery.com>
Date:   Sat Jan 10 00:08:00 2015 +0000

    Support Newlib as libc++'s C library [cstdio part, part 2]
    
    Wrappers for clearerr, feof, ferror (which newlib implements as macros).
    
    http://reviews.llvm.org/D5420
    
    llvm-svn: 225563

I strongly think that providing these functions as macros should be considered a bug on these C Standard Libraries, since it makes them unusable with C++ (without such workarounds). Can someone confirm whether newlib and glibc still define those as macros? If not, I suggest we remove these workarounds. If so, then I think we should pressure them to fix this and then eventually remove the workarounds.

This revision now requires changes to proceed.Jan 13 2021, 7:15 AM
ldionne changed the repository for this revision from rCXX libc++ to rG LLVM Github Monorepo.Jan 13 2021, 7:17 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 13 2021, 7:17 AM

The C11 standard states (in section 7.21.7.5 The getc function):

2 The getc function is equivalent to fgetc, except that if it is implemented as a macro, it may evaluate stream more than once, so the argument should never be an expression with side effects.

The C11 standard states (in section 7.21.7.5 The getc function):

2 The getc function is equivalent to fgetc, except that if it is implemented as a macro, it may evaluate stream more than once, so the argument should never be an expression with side effects.

Hmm, thanks for the quote. I didn't know it was acknowledged like that.

However, that is C, and I would still claim that an implementation that defines those as macros is C++-unfriendly. I actually don't even know how things work if getc is a macro: how is using ::getc; ever going to work? If getc is a macro, then arguably ::getc (after undefining the macro) isn't pointing to a valid declaration, unless they decided to provide both a function ::getc *and then* to define a macro with the same name. In any case, that sounds twisted. Am I missing something?

NetBSD uses macros too, but undefines them inside the header for __cplusplus. The proposed patch looks fine. This is a common source of portability issues, the stdio macros.

I still don't understand how things work if we #undef those macros. If getc was a macro, then arguably ::getc (after undefining the macro) isn't pointing to a valid declaration, unless they decided to provide both a function ::getc *and then* to define a macro with the same name. How does that work? @brad

These symbols are usually macros + libc calls.

ldionne requested changes to this revision.Jan 18 2021, 1:01 PM

These symbols are usually macros + libc calls.

I think we're talking past each other. What I'm asking is this. Imagine the libc headers look like this:

extern int __getc_implementation(FILE*);
#define getc(stream) __getc_implementation(stream)

Now, we go and undefined the getc macro in our headers. When we do using ::getc, what happens? An error, because the C library doesn't provide a declaration for ::getc. The only way I can see this working is if the C library looks like this instead:

extern int getc(FILE*); // actually provide a declaration for getc

extern int __getc_implementation(FILE*);
#define getc(stream) __getc_implementation(stream) // but then define getc to being some macro

Now, if when we undefine getc in libc++, using ::getc will refer to the ::getc defined before the C library defined the macro. That's the only case in which I can imagine the #undefs in our stdio.h to make sense.

But is that really what your implementation does? If so, why? It just looks twisted to me, but there must be a reason?

ldionne commandeered this revision.Sep 7 2023, 10:30 AM
ldionne edited reviewers, added: brad; removed: ldionne.

[Github PR transition cleanup]

Commandeering to land since regardless of my lack of understanding of how this works, we do it for other functions so it makes sense to do it for putchar and getchar too.

Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2023, 10:30 AM
ldionne accepted this revision.Sep 7 2023, 10:32 AM
This revision is now accepted and ready to land.Sep 7 2023, 10:32 AM