Re: [PATCH bpf-next v1 7/8] selftests/bpf: Fix using stdout, stderr as struct field names

[Tony Ambardar <tony.ambardar@gmail.com>]

On Thu, Jul 25, 2024 at 01:27:03PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@gmail.com>
> >
> > Typically stdin, stdout, stderr are treated as reserved identifiers under
> > ISO/ANSI C, and a libc implementation is free to define these as macros.
> 
> Ok, wow that. Do you have a pointer to where in the standard it is
> said that stdin/stdout/stderr is some sort of reserved identifier that
> can't be used as a field name?
> 

I'll need to dig around to share some references. The short answer IIRC
is there's enough potential variation in their definitions that their
use requires care (or better avoidance).

> 
> I really don't like these underscored field names. If we have to
> rename, I'd prefer something like env.saved_stdout instead of
> env._stdout. But I'd prefer even more if musl wasn't doing this macro
> definition, of course...

OK, I'll use clearer names for a v2.

I believe the macro definitions are quite common and old, but "how"
makes a difference: specifically, using parenthesis happens to break our
.stdxxx field names.


In glibc <stdio.h> we have for example:
...
/* Standard streams.  */
extern FILE *stdin;             /* Standard input stream.  */
extern FILE *stdout;            /* Standard output stream.  */
extern FILE *stderr;            /* Standard error output stream.  */
/* C89/C99 say they're macros.  Make them happy.  */
#define stdin stdin
#define stdout stdout
#define stderr stderr
...

while in musl <stdio.h> we have:
...
extern FILE *const stdin;
extern FILE *const stdout;
extern FILE *const stderr;

#define stdin  (stdin)
#define stdout (stdout)
#define stderr (stderr)
...

which borks code in test_progs.c:
...
env.stderr = stderr;
env.stdout = stdout;
...


> 
> > This is the case in musl libc and results in compile errors when these
> > names are reused as struct fields, as with 'struct test_env' and related
> > usage in test_progs.[ch] and reg_bounds.c.
> >
> > Rename the fields to _stdout and _stderr to avoid many errors seen building
> > against musl, e.g.:
> >
> >   In file included from test_progs.h:6,
> >                    from test_progs.c:5:
> >   test_progs.c: In function 'print_test_result':
> >   test_progs.c:237:21: error: expected identifier before '(' token
> >     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
> >         |                     ^~~~~~
> >   test_progs.c:237:9: error: too few arguments to function 'fprintf'
> >     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
> >         |         ^~~~~~~
> >
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
> >  tools/testing/selftests/bpf/test_progs.c      | 66 +++++++++----------
> >  tools/testing/selftests/bpf/test_progs.h      |  8 +--
> >  3 files changed, 38 insertions(+), 38 deletions(-)
> >
> 
> [...]