A tiny Raspberry Pico shell with flash file system, vi, and c compiler.

[lurk101]

Are there break statements missing here?

Code: Select all

static const char* nextpath() {
    const char* s;
    switch (dgiter.st) {
    case 0:
        s = nextcmd();
        if (s)
            return s;
        dgiter.st = 1;
        openfil(full_path(""));
    case 1:
        s = nextfil();
        if (s)
            return s;
        dgiter.st = 2;
        openfil("/bin");
    default:
        return nextfil();
    }
}

[ejolson]

Are there break statements missing here?

Code: Select all

static const char* nextpath() {
    const char* s;
    switch (dgiter.st) {
    case 0:
        s = nextcmd();
        if (s)
            return s;
        dgiter.st = 1;
        openfil(full_path(""));
    case 1:
        s = nextfil();
        if (s)
            return s;
        dgiter.st = 2;
        openfil("/bin");
    default:
        return nextfil();
    }
}

They are meant to fall through.

[VincentARM]

No, there's no such prohibition. The MMU would need to be involved. The problem is elsewhere. You need to be more specific about 'it doesn't work.'

Indeed, there was an error in my program.
With the thumb arm6 instructions, the instructions must be aligned at the word boundary but also when calling a procedure the address passed in a register must be the address of the procedure + 1!!!
Thanks.

[lurk101]

No, there's no such prohibition. The MMU would need to be involved. The problem is elsewhere. You need to be more specific about 'it doesn't work.'

Indeed, there was an error in my program.
With the thumb arm6 instructions, the instructions must be aligned at the word boundary but also when calling a procedure the address passed in a register must be the address of the procedure + 1!!!
Thanks.

Yep, it was explained a while back here.

[jahboater]

They are meant to fall through.

Might be worth telling the reader and the compiler with /*fallthrough*/

[jamesh]

They are meant to fall through.

Might be worth telling the reader and the compiler with /*fallthrough*/

This ^^^^

[lurk101]

They are meant to fall through.

Might be worth telling the reader and the compiler with /*fallthrough*/

This ^^^^

The compiler doesn't care, but for the reader's sake I've added the comments.

It is odd that GCC will warn of all sorts of possible missteps, but not a potentially missing break statement.

[jahboater]

The compiler doesn't care, but for the reader's sake I've added the comments.

It is odd that GCC will warn of all sorts of possible missteps, but not a potentially missing break statement.

The GCC compiler does care and it most definately does warn about it:

Code: Select all

try.c: In function 'unary':
try.c:4830:9: error: this statement may fall through [-Werror=implicit-fallthrough=]
 4830 |         add_event(W_OVER, erp);
      |         ^~~~~~~~~~~~~~~~~~~~~~
try.c:4831:7: note: here
 4831 |       default:
      |       ^~~~~~~

[lurk101]

The compiler doesn't care, but for the reader's sake I've added the comments.

It is odd that GCC will warn of all sorts of possible missteps, but not a potentially missing break statement.

The GCC compiler does care and it most definately does warn about it:

Code: Select all

try.c: In function 'unary':
try.c:4830:9: error: this statement may fall through [-Werror=implicit-fallthrough=]
 4830 |         add_event(W_OVER, erp);
      |         ^~~~~~~~~~~~~~~~~~~~~~
try.c:4831:7: note: here
 4831 |       default:
      |       ^~~~~~~

Strange, I don't see them using the default SDK cmake build. Do I need to enable additional non-default compiler options?

[jahboater]

Strange, I don't see them using the default SDK cmake build. Do I need to enable additional non-default compiler options?

I think you just need -Wextra

Since fall-through is perfectly correct C code, the compiler can't reasonably complain without the warnings turned on.

If you read the man page (man gcc) there is pages of stuff about -Wimplicit-fallthrough=n
but it seems to be mostly about the syntax choices for /*fallthrough*/

[ejolson]

Strange, I don't see them using the default SDK cmake build. Do I need to enable additional non-default compiler options?

I think you just need -Wextra

Since fall-through is perfectly correct C code, the compiler can't reasonably complain without the warnings turned on.

If you read the man page (man gcc) there is pages of stuff about -Wimplicit-fallthrough=n
but it seems to be mostly about the syntax choices for /*fallthrough*/

According to Fido, compilers should never peek inside the comments. What's inside should be reserved for only dogs to read. Although I disagree with part of that, I find including compiler directives inside magic comments more likely to go wrong than having a standard syntax for preprocessor and #pragma directives.

As far as modern languages go, Go somehow surpassed Turbo Pascal for a proliferation of magic comments that are not syntax checked for errors.

[lurk101]

Strange, I don't see them using the default SDK cmake build. Do I need to enable additional non-default compiler options?

I think you just need -Wextra

Since fall-through is perfectly correct C code, the compiler can't reasonably complain without the warnings turned on.

If you read the man page (man gcc) there is pages of stuff about -Wimplicit-fallthrough=n
but it seems to be mostly about the syntax choices for /*fallthrough*/

So /*fallthrough*/ prevents the compiler from complaining. Cool, I can live with that.

Now if I could find a way to circumvent these without disabling the warning completely.

Code: Select all

/home/pi/pico-dev/pshell/src/main.c: In function 'full_path.constprop':
/home/pi/pico-dev/pshell/src/main.c:95:9: warning: '__builtin_memcmp_eq' reading 3 bytes from a region of size 0 [-Wstringop-overflow=]
     if (memcmp(name, "../", 3) != 0) {
         ^~~~~~~~~~~~~~~~~~~~~~
/home/pi/pico-dev/pshell/src/main.c:95:9: warning: '__builtin_memcmp_eq' reading 3 bytes from a regi

name is a char pointer.

[jahboater]

According to Fido, compilers should never peek inside the comments. What's inside should be reserved for only dogs to read. Although I disagree with part of that, I find including compiler directives inside magic comments more likely to go wrong than having a standard syntax for preprocessor and #pragma directives.

Its nothing to do with compilers as such.
This started long ago (probably 45 years ago or so) when static code analysis tools such as lint allowed users to provide extra information such as /*notreached*/ or /*fallthrough*/ to clarify their intentions.
This idea worked well because compilers simply ignored the comments.

More recently compilers do a decent job of static analysis (see -fanalyzer especially), and so such annotation has again become useful.
Simple compilers will, as before, ignore them.

[jahboater]

Now if I could find a way to circumvent these without disabling the warning completely.

Code: Select all

/home/pi/pico-dev/pshell/src/main.c: In function 'full_path.constprop':
/home/pi/pico-dev/pshell/src/main.c:95:9: warning: '__builtin_memcmp_eq' reading 3 bytes from a region of size 0 [-Wstringop-overflow=]
     if (memcmp(name, "../", 3) != 0) {
         ^~~~~~~~~~~~~~~~~~~~~~
/home/pi/pico-dev/pshell/src/main.c:95:9: warning: '__builtin_memcmp_eq' reading 3 bytes from a regi

name is a char pointer.

That looks bad.
Since "../" is 4 bytes long, the warning must be referring to the first argument, the "name" char * pointer.
My guess is that the compiler has proven that "name" points to something zero length or maybe is NULL, or could possibly point to something like that.

I would look very carefully at name's provenance.

[lurk101]

Now if I could find a way to circumvent these without disabling the warning completely.

Code: Select all

/home/pi/pico-dev/pshell/src/main.c: In function 'full_path.constprop':
/home/pi/pico-dev/pshell/src/main.c:95:9: warning: '__builtin_memcmp_eq' reading 3 bytes from a region of size 0 [-Wstringop-overflow=]
     if (memcmp(name, "../", 3) != 0) {
         ^~~~~~~~~~~~~~~~~~~~~~
/home/pi/pico-dev/pshell/src/main.c:95:9: warning: '__builtin_memcmp_eq' reading 3 bytes from a regi

name is a char pointer.

That looks bad.
Since "../" is 4 bytes long, the warning must be referring to the first argument, the "name" char * pointer.
My guess is that the compiler has proven that "name" points to something zero length or maybe is NULL, or could possibly point to something like that.

I would look very carefully at name's provenance.

name is a char* to a file name. It isn't NULL when later used as a file open. It checks the 1st 3 characters of a file name string and works just as expected despite the warning.

[trejan]

name is a char* to a file name. It isn't NULL when later used as a file open. It checks the 1st 3 characters of a file name string and works just as expected despite the warning.

There are a few places that are doing full_path("")?

Why memcmp instead of strcmp?

[lurk101]

name is a char* to a file name. It isn't NULL when later used as a file open. It checks the 1st 3 characters of a file name string and works just as expected despite the warning.

There are a few places that are doing full_path("")?

Why memcmp instead of strcmp?

strcmp would not match something like "../abc". I suppose I could use strncmp.

[trejan]

strcmp would not match something like "../abc". I suppose I could use strncmp.

Yeah. Sorry. I meant strncmp. Using that should get rid of the warning as it won't go past the end.

[ejolson]

strcmp would not match something like "../abc". I suppose I could use strncmp.

Yeah. Sorry. I meant strncmp. Using that should get rid of the warning as it won't go past the end.

I think the problem is memcmp may be vectorised and not stop at the first byte where the match failed. Those warnings have been there for a long time and it would be nice if they went away.

[jahboater]

I think the problem is memcmp may be vectorised and not stop at the first byte where the match failed. Those warnings have been there for a long time and it would be nice if they went away.

I suspect that such undefined behavior (reading past the end of an array) would also be picked up by run-time validation tools like valgrind or the sanitizers. Although I doubt its much of a real problem these days (no x86 segmented memory), I'm still pleased the compiler warns me - I don't like UB in my code

I agree the memcmp() overrun thing is annoying.
Where it matters I tend to use a little hand written function instead:

Code: Select all

  // memcmp() avoiding buffer overread
static pure bool
ecmp( const char *a, const char *b, long count )
{
  while( count-- )
    if( *a++ != *b++ )
      return false;
  return true;
}

[lurk101]

I think using strncmp is the correct approach, the warnings should be gone now.

[lurk101]

Had a user complain that he lost all of his files in pshell every time he loaded another application on his VGA board. He wished the file system could reside on microSD instead of flash.

Pleased to accommodate: https://github.com/lurk101/pshell.git

pshell can now be built with -DPICO_BOARD=vgaboard which uses SD for the filesystem. Still in LittleFS format though, so not Linux or Windows compatible. I may consider implementing a FAT32 filesystem on SD to provide better portability.

[DarkElvenAngel]

Had a user complain that he lost all of his files in pshell every time he loaded another application on his VGA board. He wished the file system could reside on microSD instead of flash.

Pleased to accommodate: https://github.com/lurk101/pshell.git

pshell can now be built with -DPICO_BOARD=vgaboard which uses SD for the filesystem. Still in LittleFS format though, so not Linux or Windows compatible. I may consider implementing a FAT32 filesystem on SD to provide better portability.

That's really neat does that mean you could take the SD card between Pico's and run the code? If I wanted to run that binary on a pico without pshell could that be done?

[lurk101]

Had a user complain that he lost all of his files in pshell every time he loaded another application on his VGA board. He wished the file system could reside on microSD instead of flash.

Pleased to accommodate: https://github.com/lurk101/pshell.git

pshell can now be built with -DPICO_BOARD=vgaboard which uses SD for the filesystem. Still in LittleFS format though, so not Linux or Windows compatible. I may consider implementing a FAT32 filesystem on SD to provide better portability.

That's really neat does that mean you could take the SD card between Pico's and run the code? If I wanted to run that binary on a pico without pshell could that be done?

No, the executables generated in by the compiler make function calls to pshell and pico-sk code that live in the pshell image. The SD cards are portable between vgaboards with pshell loaded, but I don't think that's what you mean.

Presently the LittleFS file system is used on the SD card, but I'm looking for a minimal FAT32 filesystem implementation that could be used for SD cards. That way you could just pop the card into your PC or laptop and have access to its contents. Beats having to use xmodem to transfer files.

On a tangent, freeing up the flash makes room for a more C99 compliant compiler, but that's a long term goal and really only applicable to pico boards with microSD capability.

[ejolson]

Had a user complain that he lost all of his files in pshell every time he loaded another application on his VGA board. He wished the file system could reside on microSD instead of flash.

Pleased to accommodate: https://github.com/lurk101/pshell.git

pshell can now be built with -DPICO_BOARD=vgaboard which uses SD for the filesystem. Still in LittleFS format though, so not Linux or Windows compatible. I may consider implementing a FAT32 filesystem on SD to provide better portability.

That's really neat does that mean you could take the SD card between Pico's and run the code? If I wanted to run that binary on a pico without pshell could that be done?

No, the executables generated in by the compiler make function calls to pshell and pico-sk code that live in the pshell image. The SD cards are portable between vgaboards with pshell loaded, but I don't think that's what you mean.

Presently the LittleFS file system is used on the SD card, but I'm looking for a minimal FAT32 filesystem implementation that could be used for SD cards. That way you could just pop the card into your PC or laptop and have access to its contents. Beats having to use xmodem to transfer files.

On a tangent, freeing up the flash makes room for a more C99 compliant compiler, but that's a long term goal and really only applicable to pico boards with microSD capability.

I'd suggest using the same FAT library as used by BBC Basic.

https://github.com/Memotech-Bill/PicoBB