On Tue, Apr 25, 2023 at 7:29 AM Florian Weimer <[email protected]> wrote:
>
> In future compilers, -Wno-implicit-function-declaration may not bring
> back support for implicit function declarations, a feature that was
> removed from the C language in C99. Instead, declare the yylex
> functions using the appropriate argument types. The solution chosen
> here is not ideal because the prototypes are not verified against
> the function implementations, but the way bison and flex generate
> code make it difficult to share the prototype.
>
> This change should prevent build failures with future compilers which
> no longer support implicit function declarations by default.
>
> Signed-off-by: Florian Weimer <[email protected]>
This seems non-standard. Isn't the issue that we're not including the
appropriate <...>-flex.h ? The use of yylex for the function name
obfuscates this a bit. For example:
pmu-flex.h:
...
#ifdef yylex
#define perf_pmu_lex_ALREADY_DEFINED
#else
#define yylex perf_pmu_lex
#endif
...
/* Default declaration of generated scanner - a define so the user can
* easily add parameters.
*/
#ifndef YY_DECL
#define YY_DECL_IS_OURS 1
extern int yylex \
(YYSTYPE * yylval_param , yyscan_t yyscanner);
#define YY_DECL int yylex \
(YYSTYPE * yylval_param , yyscan_t yyscanner)
#endif /* !YY_DECL */
...
Thanks,
Ian
> ---
> tools/perf/util/Build | 2 +-
> tools/perf/util/expr.y | 1 +
> tools/perf/util/parse-events.y | 1 +
> tools/perf/util/pmu.y | 2 ++
> 4 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 918b501f9bd8..4a3ec6b0bbf6 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -283,7 +283,7 @@ CFLAGS_expr-flex.o += $(flex_flags)
> bison_flags := -DYYENABLE_NLS=0
> BISON_GE_35 := $(shell expr $(shell $(BISON) --version | grep bison | sed -e 's/.\+ \([0-9]\+\).\([0-9]\+\)/\1\2/g') \>\= 35)
> ifeq ($(BISON_GE_35),1)
> - bison_flags += -Wno-unused-parameter -Wno-nested-externs -Wno-implicit-function-declaration -Wno-switch-enum -Wno-unused-but-set-variable -Wno-unknown-warning-option
> + bison_flags += -Wno-unused-parameter -Wno-nested-externs -Wno-switch-enum -Wno-unused-but-set-variable -Wno-unknown-warning-option
> else
> bison_flags += -w
> endif
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 635e562350c5..e5731da2e3d9 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -53,6 +53,7 @@
> %destructor { ids__free($$.ids); } <ids>
>
> %{
> +int expr_lex(YYSTYPE *, void *);
> static void expr_error(double *final_val __maybe_unused,
> struct expr_parse_ctx *ctx __maybe_unused,
> bool compute_ids __maybe_unused,
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index be8c51770051..9dbab19885f3 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -18,6 +18,7 @@
> #include "parse-events.h"
> #include "parse-events-bison.h"
>
> +int parse_events_lex(YYSTYPE *, YYLTYPE *, void *);
> void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg);
>
> #define ABORT_ON(val) \
> diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
> index e675d79a0274..8405f9e6535c 100644
> --- a/tools/perf/util/pmu.y
> +++ b/tools/perf/util/pmu.y
> @@ -16,6 +16,8 @@ do { \
> YYABORT; \
> } while (0)
>
> +int perf_pmu_lex(void);
> +
> %}
>
> %token PP_CONFIG
>
> base-commit: 173ea743bf7a9eef04460e03b00ba267cc52aee2
>
* Ian Rogers:
> On Tue, Apr 25, 2023 at 7:29 AM Florian Weimer <[email protected]> wrote:
>>
>> In future compilers, -Wno-implicit-function-declaration may not bring
>> back support for implicit function declarations, a feature that was
>> removed from the C language in C99. Instead, declare the yylex
>> functions using the appropriate argument types. The solution chosen
>> here is not ideal because the prototypes are not verified against
>> the function implementations, but the way bison and flex generate
>> code make it difficult to share the prototype.
>>
>> This change should prevent build failures with future compilers which
>> no longer support implicit function declarations by default.
>>
>> Signed-off-by: Florian Weimer <[email protected]>
>
> This seems non-standard. Isn't the issue that we're not including the
> appropriate <...>-flex.h ? The use of yylex for the function name
> obfuscates this a bit. For example:
>
> pmu-flex.h:
> ...
> #ifdef yylex
> #define perf_pmu_lex_ALREADY_DEFINED
> #else
> #define yylex perf_pmu_lex
> #endif
> ...
> /* Default declaration of generated scanner - a define so the user can
> * easily add parameters.
> */
> #ifndef YY_DECL
> #define YY_DECL_IS_OURS 1
>
> extern int yylex \
> (YYSTYPE * yylval_param , yyscan_t yyscanner);
>
> #define YY_DECL int yylex \
> (YYSTYPE * yylval_param , yyscan_t yyscanner)
> #endif /* !YY_DECL */
> ...
I can try to get this to work. We have to change tools/perf/util/Build
quite extensively because
$(OUTPUT)util/parse-events-flex.c $(OUTPUT)util/parse-events-flex.h: util/parse-events.l $(OUTPUT)util/parse-events-bison.c
$(call rule_mkdir)
$(Q)$(call echo-cmd,flex)$(FLEX) -o $(OUTPUT)util/parse-events-flex.c \
--header-file=$(OUTPUT)util/parse-events-flex.h $(PARSER_DEBUG_FLEX) $<
defines two independent rules which may run in parallel and clobber each
other's two output output files. This becomes an issue once we add the
required
$(OUTPUT)parse-events-bison.o : $(OUTPUT)parse-events-flex.h
so that flex runs before compiling the bison output file, and the header
is actually available for inclusion when it's required. Maybe it's
possible to avoid the issue by depending on
$(OUTPUT)parse-events-flex.c.
Anyway, the next problem is that if we include expr-flex.h too early,
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 635e562350c5..41c36dc3cf63 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -7,6 +7,7 @@
#include "util/debug.h"
#define IN_EXPR_Y 1
#include "expr.h"
+#include "expr-flex.h"
%}
%define api.pure full
we get this:
In file included from util/expr.y:10: util/expr-flex.h:496:1: error: unknown type name 'YYSTYPE'
496 |
| ^
util/expr-flex.h:498:19: error: unknown type name 'YYSTYPE'
498 |
| ^
util/expr-flex.h:546:17: error: unknown type name 'YYSTYPE'
546 | extern int yylex \
| ^~
util/expr-bison.c: In function 'expr_parse':
util/expr-bison.c:69:25: error: implicit declaration of function 'expr_lex'
69 | #define yylex expr_lex
| ^~~~~~~~
util/expr-bison.c:1191:16: note: in expansion of macro 'yylex'
1191 | yychar = yylex (&yylval, scanner);
| ^~~~~
But expr.y seems to be the only one suffering from this, and moving the
#include a bit later appears to fix it:
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 635e562350c5..99581193ca4c 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -53,6 +53,8 @@
%destructor { ids__free($$.ids); } <ids>
%{
+#include "expr-flex.h"
+
static void expr_error(double *final_val __maybe_unused,
struct expr_parse_ctx *ctx __maybe_unused,
bool compute_ids __maybe_unused,
It works with my bison/flex combination at least, but as a change, it's
going to be a little bit risiker (both the Makefile part, and the
#include placement and general header compatibility).
I'm going to send a v2, then you can look at both and see which one you
prefer.
Thanks,
Florian