On Mon, Apr 13, 2020 at 12:32 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> I _feel_ like the fix to that should be that the only thing that
> creates the actual symbol is the label definition, and that the goto
> should only ever use the 'ident' and we'd tie the two together later.
Actually, how about something like this?
I've not signed off on these patches, and the commit logs are
questionable, but part of that is that the two first ones are just
quick-and-dirty versions of your rename cleanups.
The third patch is the serious one, which shows what I think might be
the solution to the odd scoping rules for labels.
Basically, we always scope labels - but if a label is _used_ but not
defined in an inner label, when we close the label scope, we move it
out to the next level.
But a defined label is never moved out, and when we define it, we
require that any previous use was in the same scope (where "same
scope" might have been an inner scope that was moved out).
I think it gets the semantics right, and it's actually fairly simple.
But it has very little testing, so this is more of a "how about
something like this" than a serious submission.
If you test it, and fix up the warnings and error cases (like the
other patches in your series did), you are more than welcome to take
credit and authorship for this.
I just felt that the best way to describe (and do _some_ testing) my
idea was to have a quick implementation to show what I mean.
And by "_some_ testing" I literally mean "almost no testing at all". I
didn't even run this on the kernel tree. I just used one stipid small
test-case for this, and when it gave the warning I wanted, I said
"good enough" and sent this email out ;)
Linus
From 455a1c70c1b2d8ecfb267dda8a6a5f6f8baf4233 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Mon, 13 Apr 2020 14:06:59 -0700
Subject: [PATCH 1/3] Rename 'symbol_scope' to 'block_scope'
The actual scope variable was already named that way, but the begin/end
functions inexplicably weren't.
---
expression.c | 4 ++--
parse.c | 12 ++++++------
scope.c | 4 ++--
scope.h | 4 ++--
4 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/expression.c b/expression.c
index 5b9bddfe..40141a5e 100644
--- a/expression.c
+++ b/expression.c
@@ -71,9 +71,9 @@ struct token *parens_expression(struct token *token, struct expression **expr, c
struct statement *stmt = alloc_statement(token->pos, STMT_COMPOUND);
*expr = e;
e->statement = stmt;
- start_symbol_scope();
+ start_block_scope();
token = compound_statement(token->next, stmt);
- end_symbol_scope();
+ end_block_scope();
token = expect(token, '}', "at end of statement expression");
} else
token = parse_expression(token, expr);
diff --git a/parse.c b/parse.c
index a29c67c8..9d11309e 100644
--- a/parse.c
+++ b/parse.c
@@ -2222,7 +2222,7 @@ static void start_iterator(struct statement *stmt)
{
struct symbol *cont, *brk;
- start_symbol_scope();
+ start_block_scope();
cont = alloc_symbol(stmt->pos, SYM_NODE);
bind_symbol(cont, &continue_ident, NS_ITERATOR);
brk = alloc_symbol(stmt->pos, SYM_NODE);
@@ -2237,7 +2237,7 @@ static void start_iterator(struct statement *stmt)
static void end_iterator(struct statement *stmt)
{
- end_symbol_scope();
+ end_block_scope();
}
static struct statement *start_function(struct symbol *sym)
@@ -2282,7 +2282,7 @@ static void start_switch(struct statement *stmt)
{
struct symbol *brk, *switch_case;
- start_symbol_scope();
+ start_block_scope();
brk = alloc_symbol(stmt->pos, SYM_NODE);
bind_symbol(brk, &break_ident, NS_ITERATOR);
@@ -2302,7 +2302,7 @@ static void end_switch(struct statement *stmt)
{
if (!stmt->switch_case->symbol_list)
warning(stmt->pos, "switch with no cases");
- end_symbol_scope();
+ end_block_scope();
}
static void add_case_statement(struct statement *stmt)
@@ -2548,9 +2548,9 @@ static struct token *statement(struct token *token, struct statement **tree)
if (match_op(token, '{')) {
stmt->type = STMT_COMPOUND;
- start_symbol_scope();
+ start_block_scope();
token = compound_statement(token->next, stmt);
- end_symbol_scope();
+ end_block_scope();
return expect(token, '}', "at end of compound statement");
}
diff --git a/scope.c b/scope.c
index 420c0f5a..a0de2e0d 100644
--- a/scope.c
+++ b/scope.c
@@ -86,7 +86,7 @@ void start_file_scope(void)
block_scope = scope;
}
-void start_symbol_scope(void)
+void start_block_scope(void)
{
start_scope(&block_scope);
}
@@ -131,7 +131,7 @@ void new_file_scope(void)
start_file_scope();
}
-void end_symbol_scope(void)
+void end_block_scope(void)
{
end_scope(&block_scope);
}
diff --git a/scope.h b/scope.h
index 3cad514a..83741459 100644
--- a/scope.h
+++ b/scope.h
@@ -47,8 +47,8 @@ extern void start_file_scope(void);
extern void end_file_scope(void);
extern void new_file_scope(void);
-extern void start_symbol_scope(void);
-extern void end_symbol_scope(void);
+extern void start_block_scope(void);
+extern void end_block_scope(void);
extern void start_function_scope(void);
extern void end_function_scope(void);
--
2.24.0.158.g3fed155289
From f4d8234688c60bed83a3549072ac1709d942cdcf Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Mon, 13 Apr 2020 14:09:35 -0700
Subject: [PATCH 2/3] Rename 'function' scope as 'label' scope
The only thing that was scoped this way were labels. And 'function'
scope will soon be a mis-namer.
---
parse.c | 4 ++--
scope.c | 14 +++++++-------
scope.h | 6 +++---
symbol.c | 2 +-
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/parse.c b/parse.c
index 9d11309e..64aaf07b 100644
--- a/parse.c
+++ b/parse.c
@@ -2245,7 +2245,7 @@ static struct statement *start_function(struct symbol *sym)
struct symbol *ret;
struct statement *stmt = alloc_statement(sym->pos, STMT_COMPOUND);
- start_function_scope();
+ start_label_scope();
ret = alloc_symbol(sym->pos, SYM_NODE);
ret->ctype = sym->ctype.base_type->ctype;
ret->ctype.modifiers &= ~(MOD_STORAGE | MOD_QUALIFIER | MOD_TLS | MOD_ACCESS | MOD_NOCAST | MOD_NODEREF);
@@ -2263,7 +2263,7 @@ static struct statement *start_function(struct symbol *sym)
static void end_function(struct symbol *sym)
{
current_fn = NULL;
- end_function_scope();
+ end_label_scope();
}
/*
diff --git a/scope.c b/scope.c
index a0de2e0d..8df9a5e0 100644
--- a/scope.c
+++ b/scope.c
@@ -35,8 +35,8 @@
static struct scope builtin_scope = { .next = &builtin_scope };
-struct scope *block_scope = &builtin_scope, // regular automatic variables etc
- *function_scope = &builtin_scope, // labels, arguments etc
+struct scope *block_scope = &builtin_scope, // regular variables etc
+ *label_scope = &builtin_scope, // labels
*file_scope = &builtin_scope, // static
*global_scope = &builtin_scope; // externally visible
@@ -82,7 +82,7 @@ void start_file_scope(void)
file_scope = scope;
/* top-level stuff defaults to file scope, "extern" etc will choose global scope */
- function_scope = scope;
+ label_scope = scope;
block_scope = scope;
}
@@ -91,9 +91,9 @@ void start_block_scope(void)
start_scope(&block_scope);
}
-void start_function_scope(void)
+void start_label_scope(void)
{
- start_scope(&function_scope);
+ start_scope(&label_scope);
start_scope(&block_scope);
}
@@ -136,10 +136,10 @@ void end_block_scope(void)
end_scope(&block_scope);
}
-void end_function_scope(void)
+void end_label_scope(void)
{
end_scope(&block_scope);
- end_scope(&function_scope);
+ end_scope(&label_scope);
}
int is_outer_scope(struct scope *scope)
diff --git a/scope.h b/scope.h
index 83741459..e7c5ba05 100644
--- a/scope.h
+++ b/scope.h
@@ -34,7 +34,7 @@ struct scope {
extern struct scope
*block_scope,
- *function_scope,
+ *label_scope,
*file_scope,
*global_scope;
@@ -50,8 +50,8 @@ extern void new_file_scope(void);
extern void start_block_scope(void);
extern void end_block_scope(void);
-extern void start_function_scope(void);
-extern void end_function_scope(void);
+extern void start_label_scope(void);
+extern void end_label_scope(void);
extern void set_current_scope(struct symbol *);
extern void bind_scope(struct symbol *, struct scope *);
diff --git a/symbol.c b/symbol.c
index c2e6f0b4..c3ded5ef 100644
--- a/symbol.c
+++ b/symbol.c
@@ -707,7 +707,7 @@ void bind_symbol(struct symbol *sym, struct ident *ident, enum namespace ns)
if (ns == NS_MACRO)
scope = file_scope;
if (ns == NS_LABEL)
- scope = function_scope;
+ scope = label_scope;
bind_scope(sym, scope);
}
--
2.24.0.158.g3fed155289
From 268d3b8ef45e1cce442b7ffe534715a5510cce67 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Mon, 13 Apr 2020 14:52:26 -0700
Subject: [PATCH 3/3] Label scoping is special, and expression statements are
their own scope.
A label may be used before being defined, and you don't have to
pre-declare it. And it may be used in an inner scope from the
declaration, but not the other way around.
We work around this by simply making the rule be that as we close the
scope of a label symbol, instead of killing the symbol, we will transfer
that symbol to the outer label scope if it hasn't been defined yet.
But a label that has been defined can never transfer out.
And when defining a label, test that there wasn't some earlier use
of it in another (outer) scope.
---
expression.c | 4 ++--
parse.c | 11 +++++++++++
scope.c | 36 +++++++++++++++++++++++++++++++++++-
3 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/expression.c b/expression.c
index 40141a5e..08650724 100644
--- a/expression.c
+++ b/expression.c
@@ -71,9 +71,9 @@ struct token *parens_expression(struct token *token, struct expression **expr, c
struct statement *stmt = alloc_statement(token->pos, STMT_COMPOUND);
*expr = e;
e->statement = stmt;
- start_block_scope();
+ start_label_scope();
token = compound_statement(token->next, stmt);
- end_block_scope();
+ end_label_scope();
token = expect(token, '}', "at end of statement expression");
} else
token = parse_expression(token, expr);
diff --git a/parse.c b/parse.c
index 64aaf07b..8e238f59 100644
--- a/parse.c
+++ b/parse.c
@@ -2539,6 +2539,17 @@ static struct token *statement(struct token *token, struct statement **tree)
// skip the label to avoid multiple definitions
return statement(token, tree);
}
+ /*
+ * If the scope of the label symbol is different
+ * from the current label scope, that means that
+ * it must have been used at an outer scope.
+ *
+ * That's not ok.
+ */
+ if (s->scope != label_scope) {
+ sparse_error(stmt->pos, "label '%s' used outside label expression", show_ident(s->ident));
+ sparse_error(s->pos, "invalid use here");
+ }
stmt->type = STMT_LABEL;
stmt->label_identifier = s;
s->stmt = stmt;
diff --git a/scope.c b/scope.c
index 8df9a5e0..4b0f7947 100644
--- a/scope.c
+++ b/scope.c
@@ -136,10 +136,44 @@ void end_block_scope(void)
end_scope(&block_scope);
}
+/*
+ * Label scope is special.
+ *
+ * The scope of a label is limited to its definition, but
+ * a label can be used in an inner scope before it's defined.
+ *
+ * So when we end the label scope, we move all the non-defined
+ * labels out by one level.
+ */
void end_label_scope(void)
{
+ struct symbol *sym;
+ struct symbol_list *labels = label_scope->symbols;
+
+ label_scope->symbols = NULL;
+ label_scope = label_scope->next;
+ FOR_EACH_PTR(labels, sym) {
+
+ /*
+ * Do we have a definition for it?
+ * Ok, we're done with this label
+ */
+ if (sym->stmt) {
+ remove_symbol_scope(sym);
+ continue;
+ }
+
+ if (label_scope == &builtin_scope) {
+ warning(sym->pos, "Unused label '%s'", sym->ident->name);
+ remove_symbol_scope(sym);
+ continue;
+ }
+
+ /* Re-bind the symbol to the parent scope, we'll try again */
+ bind_scope(sym, label_scope);
+ } END_FOR_EACH_PTR(sym);
+
end_scope(&block_scope);
- end_scope(&label_scope);
}
int is_outer_scope(struct scope *scope)
--
2.24.0.158.g3fed155289
