New: Console Mode editions of BBC BASIC

for discussion of bbc basic for windows/sdl, brandy and more
dp11
Posts: 1756
Joined: Sun Aug 12, 2012 9:47 pm
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by dp11 »

I very much like cppcheck as a static analyser. It has found many bugs over the years for me, and very good at finding portability issues. Being a github project you can also use coverity which again has found bugs in projects for me. I do also like gcc 11.2 with all the extra warnings turned on . This again as found issues in projects . It is a bit noisy, but well worth tidying up the warnings. gcc11.2 for arm is available in arch linux. For windows users it is easy to have arch linux under WSL2.

cppcheck does give a number of low level issues. Like returning an uninitialised variable :

Code: Select all

timer_t StartTimer (int period)
    {
    timer_t dummy;
    add_repeating_timer_ms (period, UserTimerProc, NULL, &s_rpt_timer);
    return dummy;
    }
In bbasmb_arm_64.c should their be a break ?

Code: Select all

                  case PSB:
                        {
                        nxt () ;
                        if (strnicmp ((const char *)esi, "CSYNC", 5) == 0)
                            esi += 5;
                        else
                            error( 16, "" ) ;
                        instruction = 0xd503201f;       // C6.2.81
                        instruction |= (17 << 5) ;       // C6.2.196
                        }

                    case XPACD:
                        {
                        unsigned d = reg () ;
                        if (32 == reg_size( d ) )
                            error( 16, NULL ) ;
                        instruction = 0xdac147e0;       // C6.2.306
                        instruction |= (d & 0x1f) << 0;
                        }
                        break;
Cppcheck isn't perfect but the dev team appear to be very willing to accept bug reports and fix things.
User avatar
hoglet
Posts: 12658
Joined: Sat Oct 13, 2012 7:21 pm
Location: Bristol
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by hoglet »

Richard Russell wrote: Sat Nov 06, 2021 10:22 am It's setjmp/longjmp that's broken in GCC 10.3.0 on (64-bit) Windows, so at the very least it suggests that the GCC test suite doesn't have good coverage of this area. I'm not saying it's definitely a compiler bug, but that may increase the likelihood that it is.
I also wondered if it were the setjmp/longjmp bug, so I've revered to GCC Arm Embedded 7.3.1 and unfortunately the behaviour is still the same. It's also only present when using -Os; with all other optimization options everything works fine.

Here's another example of it breaking:
Screenshot from 2021-11-06 10-49-17.png
I can't get anything to execute when entered with a line number:
Screenshot from 2021-11-06 10-56-25.png
When I was building with GCC 10.3 I enabled additional warnings (-Wall -Wextra) and this is a summary of what I got

Code: Select all

      1  warning: 'addr' may be used uninitialized in this function [-Wmaybe-uninitialized]
      1  warning: argument 'prompt' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]
      1  warning: array subscript 136960 is outside array bounds of 'char[1]' [-Warray-bounds]
      4  warning: array subscript 4 is outside array bounds of 'unsigned int[1]' [-Warray-bounds]
      1  warning: comparison is always false due to limited range of data type [-Wtype-limits]
      2  warning: comparison is always true due to limited range of data type [-Wtype-limits]
      6  warning: comparison of integer expressions of different signedness: 'int' and 'heapptr' {aka 'unsigned int'} [-Wsign-compare]
      3  warning: comparison of integer expressions of different signedness: 'int' and 'size_t' {aka 'unsigned int'} [-Wsign-compare]
     72  warning: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
      2  warning: comparison of integer expressions of different signedness: 'long long int' and 'long long unsigned int' [-Wsign-compare]
      1  warning: comparison of integer expressions of different signedness: 'long long unsigned int' and 'int' [-Wsign-compare]
      8  warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare]
      1  warning: comparison of unsigned expression in '>= 0' is always true [-Wtype-limits]
      1  warning: 'dummy' is used uninitialized in this function [-Wuninitialized]
      1  warning: initialized field overwritten [-Woverride-init]
      1  warning: 'lfs_mlist_isopen' defined but not used [-Wunused-function]
      2  warning: ordered comparison of pointer with null pointer [-Wextra]
      1  warning: 'SetLoadDir' defined but not used [-Wunused-function]
     23  warning: this statement may fall through [-Wimplicit-fallthrough=]
This one in particular is with understanding:

Code: Select all

      4  warning: array subscript 4 is outside array bounds of 'unsigned int[1]' [-Warray-bounds]
It happens here, for example, and three other places all concerning prand:

Code: Select all

/home/dmb/atom/pico/bbc_basic_test/BBCSDL-bill/src/bbeval.c:558:2: warning: array subscript 4 is outside array bounds of 'unsigned int[1]' [-Warray-bounds]
  558 |  *(unsigned char*)(&prand + 1) = (ecx & ~1) | (edx & 1) ; // Preserve bits 1-7
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Which is in rnd():

Code: Select all

// Return a pseudo-random integer:
unsigned int rnd (void)
{
	unsigned int ecx = *(unsigned char*)(&prand + 1) ;
	unsigned int edx = prand ;
	unsigned int eax = (edx >> 1) | (ecx << 31) ;
	*(unsigned char*)(&prand + 1) = (ecx & ~1) | (edx & 1) ; // Preserve bits 1-7
	eax = eax ^ (edx << 12) ;
	edx = eax ^ (eax >> 20) ;
	prand = edx ;
	return edx ;
}
I suspect there is a genuine issue here, in that prand is defined as:

Code: Select all

unsigned int edx = prand ;
The address (&prand + 1) is whatever is stored after &prand (i.e. the + is adding 4 to the address in this case)
Looking at the memory layout:

Code: Select all

200008b4 D moutrp
200008b8 D errlin
200008bc D prand
200008c1 D vwidth
200008c2 D errnum
This will be accessing vwidth.

I doubt this is related the the issue at hand, but it does seem to be a genuine bug.

Dave
dp11
Posts: 1756
Joined: Sun Aug 12, 2012 9:47 pm
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by dp11 »

@hoglet : if you have a moment with 10.3 add the static analyser and -flto to the command line and see if that spots anything.
User avatar
hoglet
Posts: 12658
Joined: Sat Oct 13, 2012 7:21 pm
Location: Bristol
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by hoglet »

dp11 wrote: Sat Nov 06, 2021 11:24 am @hoglet : if you have a moment with 10.3 add the static analyser and -flto to the command line and see if that spots anything.
First with -fanalyzer, it seems to hit an internal compiler issue in the Pico SDK code:

Code: Select all

during IPA pass: analyzer
/home/dmb/atom/pico/bbc_basic_test/pico-sdk/src/rp2_common/hardware_flash/flash.c: In function 'flash_enable_xip_via_boot2':
/home/dmb/atom/pico/bbc_basic_test/pico-sdk/src/rp2_common/hardware_flash/flash.c:44:6: internal compiler error: in get_or_create_mem_ref, at analyzer/region-model.cc:6932
   44 |     ((void (*)(void))boot2_copyout+1)();
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Please submit a full bug report,
with preprocessed source if appropriate.
See <https://gcc.gnu.org/bugs/> for instructions.
Prior to this there were three possible leaks (-Wanalyzer-malloc-leak) identified, again all in the Pico SDK code.

Second, with -flto, it exits with an error. Here's the log:
log_flto.txt
(21.71 KiB) Downloaded 39 times
This is what caused the error:

Code: Select all

/home/dmb/atom/pico/bbc_basic_test/BBCSDL-bill/mcu/pico/../../include/BBC.h:296:23: error: global register variable follows a function definition
  296 | register signed char *esi asm ("r10") ; // Program pointer
      |                       ^
lto-wrapper: fatal error: /home/dmb/atom/PiTubeDirect/not_in_git/gcc-arm-none-eabi-10.3-2021.07/bin/arm-none-eabi-g++ returned 1 exit status
In BBC.h there are some global register variables defined:

Code: Select all

// Register globals:
#ifdef __llvm__
extern signed char *esi ;		// Program pointer
extern heapptr *esp ;			// Stack pointer
#else
#ifdef __i386__
register signed char *esi asm ("esi") ;	// Program pointer
register heapptr *esp asm ("edi") ;	// Stack pointer
#endif
#ifdef __arm__
register signed char *esi asm ("r10") ;	// Program pointer
register heapptr *esp asm ("r11") ;	// Stack pointer
#endif
#ifdef __x86_64__
register signed char *esi asm ("r12") ;	// Program pointer
register heapptr *esp asm ("r13") ;	// Stack pointer
#endif
#ifdef __aarch64__
register signed char *esi asm ("x27") ;	// Program pointer
register heapptr *esp asm ("x28") ;	// Stack pointer
#endif
#endif
If I make it so esi and esp are just normal global variables (defined for now in bbpico) then the issues with -Os on the Pico go away and everything seems to work fine (so far)

Dave
Last edited by hoglet on Sat Nov 06, 2021 1:00 pm, edited 2 times in total.
dp11
Posts: 1756
Joined: Sun Aug 12, 2012 9:47 pm
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by dp11 »

Its good that no extra errors are found.
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

hoglet wrote: Sat Nov 06, 2021 11:16 am I suspect there is a genuine issue here
No. I explicitly have -Wno-array-bounds in the relevant parts of my makefiles to suppress this spurious warning.
Looking at the memory layout:

Code: Select all

200008b4 D moutrp
200008b8 D errlin
200008bc D prand
200008c1 D vwidth
200008c2 D errnum
This will be accessing vwidth.
I doubt this is related the the issue at hand, but it does seem to be a genuine bug.
No. It's neither related to the issue at hand nor a bug. Note that vwidth - prand is 5, did you read it as 4?
User avatar
hoglet
Posts: 12658
Joined: Sat Oct 13, 2012 7:21 pm
Location: Bristol
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by hoglet »

Richard Russell wrote: Sat Nov 06, 2021 1:01 pm No. It's neither related to the issue at hand nor a bug. Note that vwidth - prand is 5, did you read it as 4?
Yes, sorry, I did mis-read that as 4.

What I had missed was that prand was defined in one of the assembler files:

Code: Select all

prand:	.byte	0,0,0,0,0	/* Current 'random' number (5 bytes) */
vwidth:	.byte	0	/* Current value of WIDTH */
And indeed, there it's clear that 5 bytes are reserved.

Returning to the original issue (-Os breaking the Pico build), it does seem this is related to the use of global register variables.

I assume that was done for performance reasons?

Using -O1 and esp/esi defined as global register variables, the speed benchmark runs at 264.36MHz:
Screenshot from 2021-11-06 13-12-15.png
Using -O1 and esp/esi defined as normal global variables, the speed benchmark runs at 246.84MHz
Screenshot from 2021-11-06 13-19-35.png
Overall it's a bit slower, but that varies test-by-test (some are faster, some are slower). Possibly I'm in danger of reading too much into a rather crude benchmark.

I still don't have a explantion of why using two global register variables (for esp and esi) causes a problem with -Os.

Dave
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

hoglet wrote: Sat Nov 06, 2021 1:21 pm I still don't have a explantion of why using two global register variables (for esp and esi) causes a problem with -Os.
It has to be a compiler/optimiser bug. The -Os optimisation is seemingly trashing one or other of the register variables, which is completely wrong. The compiler is allowed to ignore the register qualifier and treat them as regular global variables, but not to trash them!

Register variables are something of a legacy thing, supported only by GCC nowadays, and it looks as though they are not being properly respected.
When you read the rationale for them still existing at all, you'll typically find a claim that modern optimisers can do every bit as good a job except in a few special cases: "For example this may be useful in programs such as programming language interpreters that have a couple of global variables that are accessed very often"! :lol:

The speed difference you noted is an indication of just how valuable they can be, and I certainly wouldn't consider switching to regular global variables to work around a compiler bug. For the time being the answer is not to use -Os on the Pico (it's fine on every other GCC platform I have tried, so this doesn't seem to be a more widespread bug).

I suppose it should be reported somewhere. To solidify the evidence it would probably be necessary to look at the assembler code emitted by the compiler, where I expect it will be obvious that one (or more) of the register variables is being trashed.
User avatar
BigEd
Posts: 6261
Joined: Sun Jan 24, 2010 10:24 am
Location: West Country
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by BigEd »

What speed is reported for the -Os version, Dave, now that you can build it? (The ARM cores on the Pico don't have cache as such, but the execute-in-place interface does offer a 16k cache, so it's possible that there's some benefit from that. Edit: although I note there are several mirrors of the cache with different behaviours, so there's a choice there too.)
User avatar
hoglet
Posts: 12658
Joined: Sat Oct 13, 2012 7:21 pm
Location: Bristol
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by hoglet »

Richard Russell wrote: Sat Nov 06, 2021 1:44 pm It has to be a compiler/optimiser bug. The -Os optimisation is seemingly trashing one or other of the register variables, which is completely wrong. The compiler is allowed to ignore the register qualifier and treat them as regular global variables, but not to trash them!
Yes, I agree with this.
Richard Russell wrote: Sat Nov 06, 2021 1:44 pm The speed difference you noted is an indication of just how valuable they can be, and I certainly wouldn't consider switching to regular global variables to work around a compiler bug. For the time being the answer is not to use -Os on the Pico (it's fine on every other GCC platform I have tried, so this doesn't seem to be a more widespread bug).
If I switch to using r6/7 rather than r10/11 the global variables work, but the performance suffers further:
Screenshot from 2021-11-06 14-13-31.png
Richard Russell wrote: Sat Nov 06, 2021 1:44 pm I suppose it should be reported somewhere. To solidify the evidence it would probably be necessary to look at the assembler code emitted by the compiler, where I expect it will be obvious that one (or more) of the register variables is being trashed.
I don't think spotting the issue will be that easy...
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

hoglet wrote: Sat Nov 06, 2021 2:53 pm If I switch to using r6/7 rather than r10/11 the global variables work, but the performance suffers further:
Yes, this will be because many Thumb2 instructions allow direct access only to the 'low' registers (r0-r7) which makes them particularly precious, and losing two from that set for general purpose use is just too expensive.
User avatar
hoglet
Posts: 12658
Joined: Sat Oct 13, 2012 7:21 pm
Location: Bristol
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by hoglet »

BigEd wrote: Sat Nov 06, 2021 2:05 pm What speed is reported for the -Os version, Dave, now that you can build it? (The ARM cores on the Pico don't have cache as such, but the execute-in-place interface does offer a 16k cache, so it's possible that there's some benefit from that. Edit: although I note there are several mirrors of the cache with different behaviours, so there's a choice there too.)
Here's the original code (using global register variables) compiled with 10.3.1 and -O1:
Screenshot from 2021-11-06 15-04-07.png
Here's the original code (using normal global variables) compiled with 10.3.1 and -O1:
Screenshot from 2021-11-06 15-13-20.png
Here's the modified code (using normal global variables) compiled with 10.3.1 and -Os:
Screenshot from 2021-11-06 15-09-35.png
Dropping the two global register variables alone reduces the speed by 8.7%

Dropping the two global register variables and switching from -O1 to -Os reduces the speed by 17.7%.

Given this, it seems there is no reason on the Pico to use -Os over -O1.

Dave
User avatar
BigEd
Posts: 6261
Joined: Sun Jan 24, 2010 10:24 am
Location: West Country
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by BigEd »

Thanks Dave! Indeed, -Os doesn't seem to be a win here. But the adventure has given us the information about what breaks -Os on the Pico, which is I think reassuring with regard to the correctness of the compiler output - it turns out the -Os problem was not evidence of something in the code being too dangerously close to the edge of the C spec.
Soruk
Posts: 1136
Joined: Mon Jul 09, 2018 11:31 am
Location: Basingstoke, Hampshire
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by Soruk »

BigEd wrote: Sat Nov 06, 2021 4:52 pm it turns out the -Os problem was not evidence of something in the code being too dangerously close to the edge of the C spec.
It also shows that Richard's C code is nowhere near as bad as he sometimes appears to make out! (That's a positive, in case my writing isn't clear)
Matrix Brandy BASIC VI (work in progress) The Distillery (another work in progress) Note Quiz (New educational software for the BBC and modern kit)
BBC Master 128, PiTubeDirect (Pi 3B), Pi1MHz, 5.25+3.5in dual floppy.
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

Soruk wrote: Sat Nov 06, 2021 8:21 pm It also shows that Richard's C code is nowhere near as bad as he sometimes appears to make out! (That's a positive, in case my writing isn't clear)
The thing is, it's not really "my" C code, it's (largely) assembler code which has been semi-mechanically translated to C. That's advantageous in one respect: most of the assembler code is more than 20 years old (indeed much of it can be traced back 40 years) and any serious bugs have hopefully long since been eliminated.

On the other hand it unfortunately means that the structure is pretty dire. Admittedly not to the extent of translating jmp instructions into gotos, so it could be worse (although I confess there is at least one goto in the C version), but nevertheless it's nothing like the sort of code that a competent C programmer would write.

And to the extent that I'm wanting C to behave like a kind of CPU-independent assembler, issues such as the 'aliasing' one raised earlier are potentially a major hazard, since there will have been no reason to avoid referring to the same block of memory with two different pointers in the original.

One worry I have is that the way the code has been created makes it less able to be efficiently optimised, since it typically doesn't use the kind of idioms that a human coder would, and which optimisers are particularly designed to recognise. Certainly it doesn't come close to matching the speed of the assembler original, despite claims that modern optimisers can sometimes achieve that.
cmorley
Posts: 1867
Joined: Sat Jul 30, 2016 8:11 pm
Location: Oxford
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by cmorley »

Richard Russell wrote: Sat Nov 06, 2021 10:12 pm issues such as the 'aliasing' one raised earlier are potentially a major hazard, since there will have been no reason to avoid referring to the same block of memory with two different pointers in the original.
To be clear it's type-punning by way of pointer that is the major problem - referring to the same memory by pointers of two or more different types (e.g. float* or short* and int*). Breaking the "strict aliasing" (pointers of incompatible types never alias) rules leads to hard to find bugs and as Mark elaborated possible alignment issues (typically a performance hit on x86... a crash on other architectures).

Otherwise aliasing is OK in C but the compiler will not be able to perform some optimisations.

I found this fairly digestible blog post for those wanting to read more.
User avatar
BigEd
Posts: 6261
Joined: Sun Jan 24, 2010 10:24 am
Location: West Country
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by BigEd »

(Thanks for the blog link!)

I think the point is nicely made over on the other forum, that this exercise of porting BBC Basic to a new microcontroller (and a slightly different CPU architecture) has actually proceeded quite well, finding and fixing a few problems, but not taking too much effort from two or three part-time collaborators. The result is a new language for that microcontroller, and an incremental improvement in the portability of this venerable codebase.

It's a great result, and the performance is fine (it might even improve with further attention), and there's always the assembler built-in too. A very accessible programming environment!
RobMcK
Posts: 20
Joined: Fri Nov 09, 2018 5:05 pm
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by RobMcK »

Richard Russell wrote: Sat Nov 06, 2021 1:44 pm
hoglet wrote: Sat Nov 06, 2021 1:21 pm I still don't have a explantion of why using two global register variables (for esp and esi) causes a problem with -Os.
It has to be a compiler/optimiser bug. The -Os optimisation is seemingly trashing one or other of the register variables, which is completely wrong. The compiler is allowed to ignore the register qualifier and treat them as regular global variables, but not to trash them!
I don't think that it is necessarily a compiler / optimiser bug, I'd be more inclined to suspect that it is undefined behaviour.

The compiler is allowed to reuse the registers which are allocated as global variables in (some parts of) the program provided that the original values are restored on exit.

Problems arise when code which does not know about the global register assignment calls code that does because the code which does not know about the global registers may have used those registers for internal variables will restore the original values on exit.

Things can get worse if function exit code is skipped due to the use of setjump / longjump.

Another thing to watch is signal handlers containing / calling code which assumes that the global variable registers are setup correctly as the may not be because the signal was generated when code which does not know about the global register assignment was executing.
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

cmorley wrote: Sun Nov 07, 2021 5:36 am alignment issues (typically a performance hit on x86... a crash on other architectures).
Nowadays all common 'desktop grade' CPU architectures support unaligned accesses in hardware: x86 of course, but also ARMv6 and later; ARMv8 seems as tolerant as x86 in my experience. The performance hit is often minimal.

Which is a good thing when you consider how many legacy data structures are internally unaligned; such as the BMP file format. And of course tokenised BBC BASIC programs have unaligned (ushort) line numbers!
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

RobMcK wrote: Sun Nov 07, 2021 10:14 am I'd be more inclined to suspect that it is undefined behaviour.
I disagree.
Problems arise when code which does not know about the global register assignment calls code that does
This shouldn't cause problems so long as the global register variables used are 'callee-saved' in the ABI. Inter-module calls (which is the only situation in which it can happen) should always follow the ABI, so the global register variables shouldn't be trashed. There are edge cases (such as rare examples of library functions with callbacks) but they do not affect BBC BASIC.
Things can get worse if function exit code is skipped due to the use of setjump / longjump.
The GCC documentation describes how setjmp/longjmp behave with global register variables. BBC BASIC does not rely on them being preserved in that situation, since longjmp is only used for error handling or on return to immediate mode.
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

cmorley wrote: Sun Nov 07, 2021 5:36 am referring to the same memory by pointers of two or more different types
Assembly language code has no concept of pointer 'type'. So in translating it to C, pointers were almost invariably declared as void* (helped by the GCC extension that sizeof(void) is 1) with casts used extensively to keep the compiler happy when dereferencing them. This would not be considered good practice for code written by a human!
RobMcK
Posts: 20
Joined: Fri Nov 09, 2018 5:05 pm
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by RobMcK »

Richard Russell wrote: Sun Nov 07, 2021 11:02 am
Problems arise when code which does not know about the global register assignment calls code that does
This shouldn't cause problems so long as the global register variables used are 'callee-saved' in the ABI. Inter-module calls (which is the only situation in which it can happen) should always follow the ABI, so the global register variables shouldn't be trashed. There are edge cases (such as rare examples of library functions with callbacks) but they do not affect BBC BASIC.
I don't believe that the global registers are being trashed per-se.

Looking at the fault reported -

Code: Select all

PRINT PI
repeats infinitely.

One way this could happen is:-

1. function with global registers calls a function which uses the standard ABI.
2. Standard ABI code saves registers and potentially does some work.
3. A function which expects the global registers is called.
4. This function uses and modifies the global variables.
5. This function returns.
6. The standard ABI function tries to use the registers which overlap with the global registers.
7. The standard ABI function restores the saved registers and returns.

The initial function will not see the changes to the global variables.

Undefined behaviour could happen at step 4, or step 6.

This could explain the reported fault if, when the interpreter gets to the end of the line, it then has its position reset by the return from a standard ABI function.


Having a quick look at the source, I cannot see where the global registers are setup in bbpico.c. This means that any code in bbpico.c which calls the main BBC code follows the steps outlined above when it calls the main BBC BASIC code.
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

RobMcK wrote: Mon Nov 08, 2021 8:08 am One way this could happen is:-
1. function with global registers calls a function which uses the standard ABI.
2. Standard ABI code saves registers and potentially does some work.
3. A function which expects the global registers is called.
I'm not following your explanation I'm afraid. In step 3, where is the "function which expects the global registers" being called from? Obviously it can't be called from within a "function which uses the standard ABI" since typically that won't work.

If you use global register variables at all you have to partition your project into a subset of modules/functions which know about the global register variables (because they are declared in a header, for example) and a subset of modules/functions which don't. Whilst it's allowed for a function in the first set to call a function in the second set (so long as the global registers chosen are callee-saved) it is not allowed for a function in the second set to call one in the first set.

Fortunately we have Acorn to thank for it being almost impossible to get that wrong in the case of BBC BASIC, because the first subset corresponds directly to what would be the 'language' in a BBC Micro (&8000 - &BFFF) and the second subset to what would be the MOS (&C000 - &FFFF). Functions in the language can call functions in the MOS, but functions in the MOS cannot call functions in the language.

In terms of modules in my Console Mode editions, the 'language' subset consists of bbmain.c, bbexec.c, bbeval.c and bbasmb_xxx.c (all of which know about the global register variables because they're declared in BBC.h) whereas the 'MOS' subset consists of bbccon.c (bbpico.c in the case of the Pico edition) and bbccos.c. I've again reproduced below the architecture diagram that I've published here before.

So I'm not sure whether I'm misunderstanding your explanation, or you're not understanding how the global register variables work in BBC BASIC. I'm using them for precisely the purpose that the GNU documentation says they can be useful for. I use them on every platform on which they are supported: 32-bit and 64-bit Windows, 32-bit and 64-bit (x86) Linux, 32-bit and 64-bit Raspberry Pi, Android and the Pico.

Only on the Pico has there been the slightest indication of a problem, and only then with one specific optimisation setting (-Os). It seems to me that all the evidence points to it being a compiler/optimiser bug, and although one would hope that is uncommon, I think it's more likely in the case of a rarely-used (and unique to GCC) feature of the language.

bbcsdl_architecture.png
Having a quick look at the source, I cannot see where the global registers are setup in bbpico.c.
They're not: bbpico.c is in the MOS subset, not the language subset. The only functions it calls in the interpreter are ones that do not use the global variables.
RobMcK
Posts: 20
Joined: Fri Nov 09, 2018 5:05 pm
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by RobMcK »

Richard Russell wrote: Mon Nov 08, 2021 9:37 am
RobMcK wrote: Mon Nov 08, 2021 8:08 am One way this could happen is:-
1. function with global registers calls a function which uses the standard ABI.
2. Standard ABI code saves registers and potentially does some work.
3. A function which expects the global registers is called.
I'm not following your explanation I'm afraid. In step 3, where is the "function which expects the global registers" being called from? Obviously it can't be called from within a "function which uses the standard ABI" since typically that won't work.

If you use global register variables at all you have to partition your project into a subset of modules/functions which know about the global register variables (because they are declared in a header, for example) and a subset of modules/functions which don't. Whilst it's allowed for a function in the first set to call a function in the second set (so long as the global registers chosen are callee-saved) it is not allowed for a function in the second set to call one in the first set.
I think that we are basically talking about the same technical details but are making a differing assumptions.

You seem to be saying "It does not happen because the code has been carefully written so that no functions in the second set calls one in the first set" and I'm trying to say "To me the evidence points to the fact that a function in the second set is calling one in the first set."

I guess careful examination of the call tree will determine if it could happen. Unfortunately I do not currently have any time to do that.
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

RobMcK wrote: Mon Nov 08, 2021 1:01 pm You seem to be saying "It does not happen because the code has been carefully written so that no functions in the second set calls one in the first set" and I'm trying to say "To me the evidence points to the fact that a function in the second set is calling one in the first set."
I don't make the claim solely because "the code has been carefully written" but also because on all platforms except the Pico, and with all optimisation options except -Os, everything works as expected. The likelihood that this would be the case if a function using the standard ABI was accidentally calling a function accessing the global register variables is small, in my opinion.

It's relevant that the two global register variables contain the BASIC 'program pointer' esi and the BASIC 'stack pointer' esp. So for your assertion to be true, a function outside the interpreter would have to be calling a function which needs to access the BASIC program or stack pointer. Why would that ever be the case?

The only functions inside the 'language' that are ever called from outside, in my BASICs, are text() (which prints a NUL-terminated string), crlf() (which prints a newline) and error() (which aborts with an error). The first two do not (and have no reason to) access the global register variables, and the third never returns: it exits via longjmp() so expects the global register variables to be destroyed anyway.

Your assertion seems to be based solely on the assumption that GCC is extremely unlikely to have a bug whereas my code is much more likely to. Whilst that is probably true, it is not "evidence" of anything. If you have any actual evidence to support your assertion, such as being able to identify a function call which does not preserve the global register variables when it should, please present it.
User avatar
SKS1
Posts: 324
Joined: Sat Sep 19, 2020 12:04 am
Location: Highland Perthshire
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by SKS1 »

No 'careful examination' is needed - it should be easy to attempt to link bbpico.o without the interpreter and see what symbols are unsatisfied.
Miserable old curmudgeon who still likes a bit of an ARM wrestle now and then. Pi 4, 3, ARMX6, SA Risc PC, A540, A440
User avatar
hoglet
Posts: 12658
Joined: Sat Oct 13, 2012 7:21 pm
Location: Bristol
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by hoglet »

I've setup the debugging environment for the Pico that comprises:
- a second Pico running PicoProbe
- OpenOCD
- GDB
- Visual Studio Code to provide a GUI over GDB

With this I've been able to single-step through the execution of PRINT PI and see where the global register variable issue actually is.

The problem turns out to be with the code produced by the compiler for item()

When -Os is used, item() looks like:

Code: Select all

1000aca8 <item>:
1000aca8:	b5f0      	push	{r4, r5, r6, r7, r14}
1000acaa:	46de      	mov	r14, r11
1000acac:	4657      	mov	r7, r10
1000acae:	b580      	push	{r7, r14}
....
.... body of item()
....
1000b074:	bcc0      	pop	{r6, r7}
1000b076:	46bb      	mov	r11, r7
1000b078:	46b2      	mov	r10, r6
1000b07a:	bdf0      	pop	{r4, r5, r6, r7, r15}
1000b07c:	f001 fa86 	bl	1000c58c <itemi>
The value of r10 (the global variable used for esi) is saved to the stack, and restored at the end. Consequently, any changes made to esi within item() will be lost. (i.e. PRINT PI loops forever, because the esp++ done in item() to advance past the PI token is lost).

When -O1 is used the pre-amble is slightly different, and r8/9 rather than r10/11 are stacked.

Code: Select all

1000b824 <item>:
1000b824:	b5f0      	push	{r4, r5, r6, r7, r14}
1000b826:	46ce      	mov	r14, r9
1000b828:	4647      	mov	r7, r8
1000b82a:	b580      	push	{r7, r14}
....
.... body of item()
....
1000c252:	bcc0      	pop	{r6, r7}
1000c254:	46b9      	mov	r9, r7
1000c256:	46b0      	mov	r8, r6
1000c258:	bdf0      	pop	{r4, r5, r6, r7, r15}
So in the case the global variables work as intended.

Dave
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

hoglet wrote: Mon Nov 08, 2021 3:30 pm The problem turns out to be with the code produced by the compiler for item()...
The value of r10 (the global variable used for esi) is saved to the stack, and restored at the end.
So is it a compiler bug or a fault of my code, or do you not yet have enough evidence to say? It seems to me that since item() is in a module which #includes BBC.h, in which the global register variables are declared, it should not be 'saving and then restoring' one of them, and thus throwing away any legitimate changes made to the global in between.
User avatar
hoglet
Posts: 12658
Joined: Sat Oct 13, 2012 7:21 pm
Location: Bristol
Contact:

Re: New: Console Mode editions of BBC BASIC

Post by hoglet »

Richard Russell wrote: Mon Nov 08, 2021 3:47 pm So is it a compiler bug or a fault of my code, or do you not yet have enough evidence to say? It seems to me that since item() is in a module which #includes BBC.h, in which the global register variables are declared, it should not be 'saving and then restoring' one of them, and thus throwing away any legitimate changes made to the global in between.
I'm very much in agreement with you Richard.

I think it's a compiler bug, specifically the -Os optimizer not respecting the semantics of global register variables.

(I haven't used global register variables myself, so it's possible I'm missing some subtlety of their correct use)

Dave
Deleted User 9295

Re: New: Console Mode editions of BBC BASIC

Post by Deleted User 9295 »

hoglet wrote: Mon Nov 08, 2021 3:55 pm (I haven't used global register variables myself, so it's possible I'm missing some subtlety of their correct use)
I admit that they are not exactly 'extensively documented'; one thing that I've failed to find, for example, is an explicit list of which registers may, and which may not, be used for global register variables in each CPU architecture. So I've resorted to applying common sense, i.e. that global register variables must be called-saved in the ABI, and not reserved for special purposes like SP (r13) or LR (r14). But that is somewhat unsatisfying given how crucial they are to the correct functioning of the interpreter.

So in the absence of formal proof that what I'm doing is valid I resort to empirical evidence, i.e. that given the size of the codebase if I was using a register not permitted for this purpose it would have shown up much earlier, and with optimisation levels other than -Os. But I know this is rather unsatisfactory to those who take a dogmatic view, and they will probably assert that 'by definition' I must be doing something wrong, leading to undefined behaviour.

So I do feel rather in a 'no win' situation. No matter how much circumstantial evidence I produce, I cannot prove that my code isn't at fault. Therefore the only thing that my detractors will accept is discontinuing my BASICs altogether. And since I was already considering doing that, I will add this sorry affair to the list of arguments for doing so.
Post Reply

Return to “modern implementations of classic programming languages”