Page MenuHomePhabricator

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

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

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
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?