[master] ed36b6389 Defer the illegal write check a bit

[Dridi Boukelmoune]

commit ed36b6389f9dbf6962e726f0df34d59132d9241a
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Wed Dec 18 18:14:59 2019 +0100

    Defer the illegal write check a bit
    
    After the initial discussion from #3163, and looking more closely at how
    variable access is handled in subroutines I noticed a discrepancy.
    
    Setting a read only variable like obj.ttl in vcl_recv would result in
    a misleading error message explaining that it is read only, instead of
    simply not available.
    
    This change defers the illegal write check, registering unconditionally
    that the symbol was used in a set action. As a result we always get the
    correct error message but depending on whether this is happening in a
    vcl_ or custom subroutine we may either get "in subroutine" or "from
    subroutine" in the error message. A minor discrepancy probably worth
    getting rid of the prior inconsistency.
    
    This is covered by the v21 test case.

diff --git a/bin/varnishtest/tests/v00021.vtc b/bin/varnishtest/tests/v00021.vtc
index dd3392ee8..ab0618f98 100644
--- a/bin/varnishtest/tests/v00021.vtc
+++ b/bin/varnishtest/tests/v00021.vtc
@@ -2,14 +2,26 @@ varnishtest "VCL compiler coverage test: vcc_xref.c vcc_var.c vcc_symb.c"
 
 varnish v1 -errvcl {Variable is read only.} {
 	backend b { .host = "127.0.0.1"; }
-	sub vcl_recv { set obj.ttl = 1 w; }
+	sub vcl_deliver { set obj.ttl = 1 w; }
 }
 
 varnish v1 -errvcl {Variable is read only.} {
 	backend b { .host = "127.0.0.1"; }
 
 	sub foo { set obj.ttl = 1 w; }
-	sub vcl_recv { call foo ; }
+	sub vcl_deliver { call foo; }
+}
+
+varnish v1 -errvcl {Not available in subroutine 'vcl_recv'.} {
+	backend b { .host = "127.0.0.1"; }
+	sub vcl_recv { set obj.ttl = 1 w; }
+}
+
+varnish v1 -errvcl {Not available from subroutine 'vcl_recv'.} {
+	backend b { .host = "127.0.0.1"; }
+
+	sub foo { set obj.ttl = 1 w; }
+	sub vcl_recv { call foo; }
 }
 
 varnish v1 -errvcl "Symbol not found" {
diff --git a/lib/libvcc/vcc_action.c b/lib/libvcc/vcc_action.c
index bcbeea9ba..8fa48b37c 100644
--- a/lib/libvcc/vcc_action.c
+++ b/lib/libvcc/vcc_action.c
@@ -127,15 +127,8 @@ vcc_act_set(struct vcc *tl, struct token *t, struct symbol *sym)
 	sym = VCC_SymbolGet(tl, SYM_VAR, SYMTAB_EXISTING, XREF_NONE);
 	ERRCHK(tl);
 	AN(sym);
-	if (sym->w_methods == 0) {
-		vcc_ErrWhere2(tl, t, tl->t);
-		if (sym->r_methods != 0)
-			VSB_printf(tl->sb, "Variable is read only.\n");
-		else
-			VSB_printf(tl->sb, "Variable cannot be set.\n");
-		return;
-	}
 	vcc_AddUses(tl, t, tl->t, sym, XREF_WRITE);
+	ERRCHK(tl);
 	type = sym->type;
 	for (ap = assign; ap->type != VOID; ap++) {
 		if (ap->type != type)
diff --git a/lib/libvcc/vcc_xref.c b/lib/libvcc/vcc_xref.c
index d2e933db3..eff883274 100644
--- a/lib/libvcc/vcc_xref.c
+++ b/lib/libvcc/vcc_xref.c
@@ -139,6 +139,9 @@ vcc_AddUses(struct vcc *tl, const struct token *t1, const struct token *t2,
 		WRONG("wrong xref use");
 
 	VTAILQ_INSERT_TAIL(&tl->curproc->uses, pu, list);
+
+	if (pu->mask == 0)
+		vcc_CheckUses(tl);
 }
 
 void
@@ -251,13 +254,40 @@ vcc_CheckAction(struct vcc *tl)
 /*--------------------------------------------------------------------*/
 
 static struct procuse *
-vcc_FindIllegalUse(const struct proc *p, const struct method *m)
+vcc_illegal_write(struct vcc *tl, struct procuse *pu, const struct method *m)
 {
-	struct procuse *pu;
 
-	VTAILQ_FOREACH(pu, &p->uses, list)
+	if (pu->mask || pu->use != XREF_WRITE)
+		return (NULL);
+
+	if (pu->sym->r_methods == 0) {
+		vcc_ErrWhere2(tl, pu->t1, pu->t2);
+		VSB_printf(tl->sb, "Variable cannot be set.\n");
+		return (NULL);
+	}
+
+	if (!(pu->sym->r_methods & m->bitval)) {
+		pu->use = XREF_READ; /* NB: change the error message. */
+		return (pu);
+	}
+
+	vcc_ErrWhere2(tl, pu->t1, pu->t2);
+	VSB_printf(tl->sb, "Variable is read only.\n");
+	return (NULL);
+}
+
+static struct procuse *
+vcc_FindIllegalUse(struct vcc *tl, const struct proc *p, const struct method *m)
+{
+	struct procuse *pu, *pw;
+
+	VTAILQ_FOREACH(pu, &p->uses, list) {
+		pw = vcc_illegal_write(tl, pu, m);
+		if (tl->err)
+			return (pw);
 		if (!(pu->mask & m->bitval))
 			return (pu);
+	}
 	return (NULL);
 }
 
@@ -268,7 +298,7 @@ vcc_CheckUseRecurse(struct vcc *tl, const struct proc *p,
 	struct proccall *pc;
 	struct procuse *pu;
 
-	pu = vcc_FindIllegalUse(p, m);
+	pu = vcc_FindIllegalUse(tl, p, m);
 	if (pu != NULL) {
 		vcc_ErrWhere2(tl, pu->t1, pu->t2);
 		VSB_printf(tl->sb, "%s from subroutine '%s'.\n",
@@ -278,6 +308,8 @@ vcc_CheckUseRecurse(struct vcc *tl, const struct proc *p,
 		vcc_ErrWhere(tl, p->name);
 		return (1);
 	}
+	if (tl->err)
+		return (1);
 	VTAILQ_FOREACH(pc, &p->calls, list) {
 		if (vcc_CheckUseRecurse(tl, pc->sym->proc, m)) {
 			VSB_printf(tl->sb, "\n...called from \"%.*s\"\n",
@@ -299,7 +331,7 @@ vcc_checkuses(struct vcc *tl, const struct symbol *sym)
 	AN(p);
 	if (p->method == NULL)
 		return;
-	pu = vcc_FindIllegalUse(p, p->method);
+	pu = vcc_FindIllegalUse(tl, p, p->method);
 	if (pu != NULL) {
 		vcc_ErrWhere2(tl, pu->t1, pu->t2);
 		VSB_printf(tl->sb, "%s in subroutine '%.*s'.",
@@ -307,6 +339,7 @@ vcc_checkuses(struct vcc *tl, const struct symbol *sym)
 		VSB_cat(tl->sb, "\nAt: ");
 		return;
 	}
+	ERRCHK(tl);
 	if (vcc_CheckUseRecurse(tl, p, p->method)) {
 		VSB_printf(tl->sb,
 		    "\n...which is the \"%s\" subroutine\n", p->method->name);