iocsh on vxWorks broken

Bug #2004463 reported by Dirk Zimoch
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
EPICS Base
Fix Committed
Critical
mdavidsaver

Bug Description

Since commit 60128ee9 "Com: separate iocsh argument splitting", iocsh is broken on VxWorks (tested with version 6.9.4.12)

Any command prints the error "Unbalanced quote."

> iocsh
epics> echo
Unbalanced quote.
epics> "echo"
Unbalanced quote.
epics> "echo
Unbalanced quote.
epics> echo bla
Unbalanced quote.
epics> echo 1 2 3
Unbalanced quote.
epics> exit
Unbalanced quote.

I have started investigating....

Revision history for this message
Dirk Zimoch (dirk.zimoch) wrote (last edit ):

The reason is in iocsh.cpp line 363:
        if (quote != EOF) {

Here, quote is 0xff but EOF is -1.

quote is initialized in line 265:
        char quote = EOF;

The problem is the usage of EOF in function split().

On any architecture, where char is unsigned (e.g. VxWorks on PPC), the initialization
        char quote = EOF;
sets quote to (char)-1 = 255.

Later, the comparison (quote != EOF) compares an unsigned char with the signed int literal -1. Thus the unsigned char gets promoted to a signed int with the same value 255 which is then then compared to -1. That is of course not equal.

The possible fixes are:
1. use signed char quote
2. use int quote
3. use 0 instead of EOF

I prefer solution 3. The attached patch has been created with commit 60128ee but can be applied to the current HEAD (bc54524) as well.

Revision history for this message
Dirk Zimoch (dirk.zimoch) wrote :

I suggest to add at least one architecture with unsigned char to the automated tests. Maybe building EPICS on Linux with -funsigned-char will do?

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> ... building EPICS on Linux with -funsigned-char ...

Adding this as a CI job seems an interesting idea. Have you tried it locally?

Revision history for this message
Dirk Zimoch (dirk.zimoch) wrote :

Not yet...

Revision history for this message
Andrew Johnson (anj) wrote :

Core Group 2/1/2023: Thanks!

This change is still in unreleased Base branch.

We agree with Dirk's analysis, and would prefer to run the tests on an architecture that has unsigned char, if necessary the use of -funsigned-char should work.

Revision history for this message
Dirk Zimoch (dirk.zimoch) wrote :

Compiling base on my Linux host with CMD_CFLAGS=-funsigned-char and CMD_CXXFLAGS=-funsigned char builds an ioc that shows the same buggy behavior (complaining about any command with "Unbalanced quote" error).

And it fails iocshTest.t:
Test Summary Report
-------------------
iocshTest.t (Wstat: 256 Tests: 19 Failed: 8)
  Failed tests: 2, 4-5, 9, 14-17

And in database/test/std/rec/:
Unbalanced quote.
iocsh Error: Break
dbAllocRecord(rec0) with ao rec_size = 0
Can't create record "rec0" of type "ao"
Error at or before ')' in path ".." file "asTest.db" line 1
Error: syntax error
Bailout called. Further testing stopped: Failed to load test database
FAILED--Further testing stopped: Failed to load test database

After applying the patch iocsh works and all tests succeed.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

The regression in https://github.com/epics-base/epics-base/pull/274 is the change from

> int quote, inword, backslash;

to

> bool inword = false;
> bool backslash = false;
> char quote = EOF;

Which in retrospect is an issue because the EOF macro is meant to be stored/compared with 'int' (eg. with 'getc()' and friends).

> 3. use 0 instead of EOF
>
> I prefer solution 3. ...

I agree.

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> The attached patch ...

Applied as 3dbc9ea26491dc676888fca091e78cddf9a8760c

I've also added a GHA job with -funsigned-char

Changed in epics-base:
status: New → Fix Committed
importance: Undecided → Critical
assignee: nobody → mdavidsaver (mdavidsaver)
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.