| Message ID | 20201026045338.55218-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | handle more graciously labels with no statement | expand |
On Sun, Oct 25, 2020 at 9:53 PM Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > This occurs currently on v5.10-rc1 because of some ifdefery. Well, sparse being more robust is good, so the patch looks sane to me.. But can you point to the actual 5.10-rc1 problem? I haven't seen the report, and afaik gcc will complain about this too ("label at end of compound statement") so I'm surprised sparse hits it.. Linus
On Mon, Oct 26, 2020 at 10:41:36AM -0700, Linus Torvalds wrote: > On Sun, Oct 25, 2020 at 9:53 PM Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > > > This occurs currently on v5.10-rc1 because of some ifdefery. > > Well, sparse being more robust is good, so the patch looks sane to me.. > > But can you point to the actual 5.10-rc1 problem? I haven't seen the > report, and afaik gcc will complain about this too ("label at end of > compound statement") so I'm surprised sparse hits it.. Well, I was surprised it wasn't caught and was ready to send a patch but it can only happen when using sparse, see below, quite ironical: drivers/scsi/qla2xxx/qla_tmpl.c:1052. 1050 } 1051 1052 bailout: 1053 #ifndef __CHECKER__ 1054 if (!hardware_locked) 1055 spin_unlock_irqrestore(&vha->hw->hardware_lock, flags); 1056 #endif 1057 } -- Luc
On Mon, Oct 26, 2020 at 1:48 PM Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > On Mon, Oct 26, 2020 at 10:41:36AM -0700, Linus Torvalds wrote: > > But can you point to the actual 5.10-rc1 problem? I haven't seen the > > report, and afaik gcc will complain about this too ("label at end of > > compound statement") so I'm surprised sparse hits it.. > > Well, I was surprised it wasn't caught and was ready to send a patch > but it can only happen when using sparse, see below, quite ironic: > drivers/scsi/qla2xxx/qla_tmpl.c:1052. > 1050 } > 1051 > 1052 bailout: > 1053 #ifndef __CHECKER__ > 1054 if (!hardware_locked) > 1055 spin_unlock_irqrestore(&vha->hw->hardware_lock, flags); > 1056 #endif > 1057 } Yeah, that "don't sparse this" pattern has been problematic before. I removed it. Bart, Arun - see commit 4525c8781ec0 ("scsi: qla2xxx: remove incorrect sparse #ifdef"). Linus
On 10/26/20 3:56 PM, Linus Torvalds wrote: > On Mon, Oct 26, 2020 at 1:48 PM Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: >> >> On Mon, Oct 26, 2020 at 10:41:36AM -0700, Linus Torvalds wrote: >>> But can you point to the actual 5.10-rc1 problem? I haven't seen the >>> report, and afaik gcc will complain about this too ("label at end of >>> compound statement") so I'm surprised sparse hits it.. >> >> Well, I was surprised it wasn't caught and was ready to send a patch >> but it can only happen when using sparse, see below, quite ironic: >> drivers/scsi/qla2xxx/qla_tmpl.c:1052. >> 1050 } >> 1051 >> 1052 bailout: >> 1053 #ifndef __CHECKER__ >> 1054 if (!hardware_locked) >> 1055 spin_unlock_irqrestore(&vha->hw->hardware_lock, flags); >> 1056 #endif >> 1057 } > > Yeah, that "don't sparse this" pattern has been problematic before. > > I removed it. > > Bart, Arun - see commit 4525c8781ec0 ("scsi: qla2xxx: remove incorrect > sparse #ifdef"). Hi Linus, Thank you for having Cc-ed me. I agree that it's better to make the kernel code compliant with the C standard than to make sparse accept non-standard code. The #ifndef __CHECKER__ / #endif that I added in 2015 in the above code (commit 8d16366b5f23) was added because at that time I didn't see a better solution. BTW, personally I'm neither enthusiast about #ifndef __CHECKER__ / #endif nor about if (expression) <locking statement>. Bart.
On Mon, Oct 26, 2020 at 4:22 PM Bart Van Assche <bvanassche@acm.org> wrote: > > The #ifndef __CHECKER__ / #endif that I added in 2015 in the above code > (commit 8d16366b5f23) was added because at that time I didn't see a > better solution. I think there are only a couple of callers, and all of them just have a constant "device_locked" argument. It should be easy to make the (I think single) use that _didn't_ lock the device just do the locking, and then all of them have "device_locked = 1", and then that argument can be removed and the whole conditional be replaced by a lockdep assert. Exactly like y9ou did in 8ae178760b23 ("scsi: qla2xxx: Simplify the functions for dumping firmware") in other words. But I didn't really look _that_ much into it, this is just from a simple "grep" thing and maybe I missed something. I just did the minimal "don't do invalid C" thing. Linus
diff --git a/parse.c b/parse.c index 31ecef0f554d..b6090d38cc61 100644 --- a/parse.c +++ b/parse.c @@ -2468,6 +2468,11 @@ static struct token *statement(struct token *token, struct statement **tree) warn_label_usage(stmt->pos, s->label_pos, s->ident); } s->stmt = stmt; + if (match_op(token, '}')) { + warning(token->pos, "statement expected after label"); + stmt->label_statement = alloc_statement(token->pos, STMT_NONE); + return token; + } return statement(token, &stmt->label_statement); } }
In C a label must precede a statement. A null statement is OK but a closing braces is not. So, catch this situation, emit a warning and continue as if a null statement was there. This occurs currently on v5.10-rc1 because of some ifdefery. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- parse.c | 5 +++++ 1 file changed, 5 insertions(+)