BeebAsm

[ctr]

I must admit I got a bit nervous on seeing your long post and expected you'd found a lot of bugs, so I'm glad it all seemed OK.

Yeah, by the time I'd finished testing I realised I had quite a long post that really didn't say very much. But thank you for making the couple of changes.

We could possibly get a cheap fix for this by having beebasm_srand() make a few calls to beebasm_rand() and discard the results before returning - what do you think?

Sounds good, it would probably only need two or three calls to give the appearance of randomness, which is all that's required.

BeebAsm was writen by Rich Talbot-Watkins but is now maintained by the members of the 'stardot' forums. The official source repository is at https://github.com/stardot/beebasm. Please post any questions or comments relating to BeebAsm on the 'Development tools' forum at stardot: http://www.stardot.org.uk/forums/viewforum.php?f=55

Sounds good. It would be sensible to use the github bug tracker because bugs reported here can easily be forgotten, but directing people here in the first instance is likely to lead to more visibility and discussion.

What do people want to do about credit for their contributions? The pattern so far seems to be to mention people by name in about.txt (e.g. Thomas Harte gets a mention under the PUTBASIC command and some people get a mention in the changelog). We could stick an "acknowledgements" section in about.txt and bung it all in there.

beebem also notes contributors for each release in the changelog. That seems to work pretty well.

[SteveF]

I've had a go at pulling a few more things onto the startdot repo's proposed-updates branch (https://github.com/stardot/beebasm/tree ... ed-updates):

• sweh's documentation fix has been merged
• Conditional assignment/-D has been merged (Charles/sweh) and is documented in about.txt
• I've implemented more verbose errors for branch out of range showing how far out of range the branch target is
• I've updated the REPORTING BUGS section of about.txt as suggested earlier
• I've made the proposed change to RANDOMIZE so the first few bytes should be affected even by small changes in the seed
• I've bumped the version number to 1.09

What I think we need to do before we call this 1.09 and merge it to master:

• I think we should add a CREDITS section to about.txt. I'm reluctant to add "Thanks to X" in the documentation for individual features because as soon as someone (e.g. me!) has a couple of contributions they are going to get redundantly thanked in multiple places, or not get credit for some of their changes. Does anyone have any objections to me doing this? I'll push the change onto proposed-updates for review before we merge to master.
• We need to be sure everyone who should get a credit does. I know about sweh and Charles, is there anyone I've overlooked? For example, quite a few people chipped in to offer thoughts on the syntax for label scope jumping over the Retro Software forum, do they want a mention?
• sweh/Charles - what exactly would you like your credit to look like? "Your Full Name <e-mail address from commit log>"? Do you want some kind of anti-spam mangling on the e-mail address? Just your stardot username? Up to you, but either let me know here or I'll just fill in something that seems reasonable in the CREDITS section and you can change it yourselves before we merge to master.
• Someone will need to do a Windows build so we can update beebasm.exe. I don't know if I can do this, although if it's just a case of installing a g++-mingw package on Ubuntu I might be able to; I haven't tried yet.
• A lot of the file headers/about.txt headers have a copyright message for Rich 2007-2012. Do we want/need to change this, at least to change 2012 to 2018? Does anyone else want to add a copyright header of their own for files they have changed?
• It would be good if people could try building their own projects with the proposed 1.09 code so we don't have to do a bug-fix 1.10 straight away.

What else have I forgotten?

Cheers.

Steve

[kieranhj]

I[*]Someone will need to do a Windows build so we can update beebasm.exe. I don't know if I can do this, although if it's just a case of installing a g++-mingw package on Ubuntu I might be able to; I haven't tried yet.

I've lost my Windows beebasm build environment but have an awesome test case in POP - over 60 separate modules assembling to about 64K of 6502 code. If v1.09 is binary identical to my current v1.08 version then I think that could be considered pretty good coverage.

The code is all in my GitHub repo if anyone wants to build master: https://github.com/kieranhj/pop-beeb. Note that you'd need to hack the version.txt file so they are identical as this is updated by the make.bat file each time it is run.

[ctr]

For me, a single mention of my stardot username in the credits file is enough. More detail is available in the commit log.

You could maybe change the copyright to something like "Copyright (C) Rich Talbot-Watkins and the contributors 2007 - 2018".

I have updated the Visual Studio project files (to add random.cpp and .h), fixed a source encoding problem in BASIC.cpp and rebuilt the exe. Can you give me (mungre on github) permission to push the changes to proposed-updates?

[SteveF]

Thanks Charles! I'm not an admin so I can't permission you but there is a sticky post in (I think; on phone so not checked) the development tools forum where you can post to request this.

Cheers.

Steve

[tricky]

The supplied VS2010 project and solution are missing the random.h and .cpp files.
When assembling one of my games, it fails with an assert, reading off the end of a std::string:

Code: Select all

double LineParser::EvaluateExpression( bool bAllowOneMismatchedCloseBracket )
{
	// Reset stacks

	m_valueStackPtr = 0;
	m_operatorStackPtr = 0;

	// Count brackets

	int bracketCount = 0;

	TYPE expected = VALUE_OR_UNARY;

	// Iterate through the expression

	while ( AdvanceAndCheckEndOfSubStatement() )
	{
		if ( expected == VALUE_OR_UNARY )
		{
			// Look for unary operator

			int matchedToken = -1;

			// Check against unary operator tokens

			for ( unsigned int i = 0; i < sizeof m_gaUnaryOperatorTable / sizeof(Operator); i++ )
			{
				const char*		token	= m_gaUnaryOperatorTable[ i ].token;
				size_t			len		= strlen( token );

				// see if token matches

				bool bMatch = true;
				for ( unsigned int j = 0; j < len; j++ )
				{
					if ( token[ j ] != toupper( m_line[ m_column + j ] ) )

m_column is already on the last character of the m_line and so adding j (!= 0) goes out of bounds.
This would have been fine as a char [] as it wouldn't have matched the '\0';
The source line is:

Code: Select all

EQUB '0' + d

Updated project and filter file attached.

PS I know it was like this before; I guess it just wasn't built with bounds checking enabled.

[ctr]

Thank you Steve and Kieran, I can push to github now.

There's a new exe on github. The last commit is a fix to the buffer overflow identified by tricky in the previous post.

[SteveF]

Thanks for fixing that!

I've built Kieran's POP code (side A only; side B doesn't seem to use beebasm on a quick glance) and the resulting SSD was identical with both beebasm 1.08 and beebasm 1.09. The 'compile.txt' file produced on standard output was identical too.

(This was on Ubuntu with a self-built binary; I didn't use the new beebasm.exe, but I had the fix for the problem tricky found in my build.)

ETA: I've also run the new 1.09 under valgrind (with the default 'memcheck' tool) while building both POP and my own STEM project and it didn't report any errors.

ETA2: I've given Charles, Stephen and myself mentions in about.txt; please shout (or just add yourself!) if I've forgotten anyone, or you'd like your mention tweaking. I haven't made the change to the copyright dates suggested by ctr yet as I'm not sure what's best, although it seems a reasonable suggestion.

ETA3: So I think the things to do before release are:

• Encourage people to test
• Decide what, if anything, to do about the copyright dates
• Put the release date in section 9 of about.txt

Did I miss anything?

[sweh]

[*]sweh/Charles - what exactly would you like your credit to look like? "Your Full Name <e-mail address from commit log>"?

"Stephen Harris <sweh@spuddy.org>" is fine.

Thanks

[SteveF]

Thanks Stephen, I've tweaked it to look like that now.

[sweh]

FWIW, I just compiled my RAM Manager and Hostfs:UPURS ROMs with this version and get identical results to 1.08. My code doesn't do anything clever, but at least there's no regressions

[SteveF]

Great, thanks for testing - it all helps to build confidence in the new version!

[simonm]

Great work guys!

[richardtoohey]

I've tried building on OpenBSD and it doesn't like M_PI but I don't have the knowledge of how to fix.

I can make it build by removing -Wold-style-cast from the Makefile.

Do not wait up for me, though, just thought I'd mention it.

Code: Select all

bash-4.4$ gmake all PLATFORM=linux
-e Making subdirectory ... objects/.
...
-e Compiling ... BASIC.cpp
-e Compiling ... symboltable.cpp
cc1plus: warnings being treated as errors
symboltable.cpp: In constructor 'SymbolTable::SymbolTable()':
symboltable.cpp:81: warning: use of old-style cast
gmake[1]: *** [Makefile.inc:227: objects/symboltable.o] Error 1
gmake: *** [Makefile.inc:183: objs] Error 2

Any line that has M_PI in it causes this error, so that line 81 is

Code: Select all

AddSymbol( "PI", M_PI );

There are a couple of M_PI references in expression.cpp.

I Googled and tried static_cast, etc., etc. but couldn't get past that warning-that-becomes-an-error. Might be the older version of GCC on here, or many other things, so don't want anyone wasting any time on it.

[sweh]

On Linux /usr/include/math.h has

Code: Select all

# define M_PI          3.14159265358979323846  /* pi */

Is there no equivalent on OpenBSD?

[ctr]

With gcc, M_PI seems to come from the platform math.h (via cmath), which on openbsd contains:

Code: Select all

#define	M_PI		((double)3.14159265358979323846) /* pi */

Which is an old-style cast, hence the error. M_PI doesn't seem to come from any standard, so it's hard to say this is wrong, but the type of a real constant is double by default so this cast does seem pointless.

You could also argue that it is the job of cmath to massage the contents of math.h into compatibility with C++. Though gcc would probably view non-standard constants as not their problem.

The right fix here seems to be for openbsd to bring their math.h into line with everybody else's. I will suggest it to them, though I'm not enormously hopeful.

You've got a workaround (remove -Wold-style-cast) so I'd be disinclined to change beebasm at this point.

Edit: I've done a bit more digging. It turns out that these casts were introduced quite recently for platforms where FLT_EVAL_METHOD==2, i.e. where all floating point literals evaluate as long doubles. The effect of the cast is to ensure that M_PI has no more precision that a double. Maybe the cleanest solution is just to redefine M_PI to something that works.

[SteveF]

I'm open to suggestions, but we could just #define (or 'const double our_pi = ...') our own constant here, if trying to use a library-provided value is causing problems. I can't see PI changing in the near future or across platforms.

[richardtoohey]

With gcc, M_PI seems to come from the platform math.h (via cmath), which on openbsd contains:

Code: Select all

#define	M_PI		((double)3.14159265358979323846) /* pi */

Which is an old-style cast, hence the error.

Ahhhh I just saw that it was defined in a header and then got bogged down in trying to fix it where it was used rather than defined. And couldn't get past that. Thank you!

I'm loathe for anyone to change BeebAsm just for this (unless anyone has a nice portable way) especially because easy enough to work-around/fudge in the Makefile if required. Rather than re-defining another M_PI.

Will also give it a whirl on FreeBSD (10.4 and 11.1) unless anyone else already has.

EDIT: Oh, I'd have to install gcc on FreeBSD ... they've moved to clang. (So has OpenBSD but gcc got pulled in for something on my desktop.) So a bit more work required.

[ctr]

I'm open to suggestions, but we could just #define (or 'const double our_pi = ...') our own constant here, if trying to use a library-provided value is causing problems. I can't see PI changing in the near future or across platforms.

I was thinking #undef/#define M_PI but you're right, a new constant would be clearer. I've created a pull request.

[SteveF]

Thanks! That change looks good to me, and it builds fine on my Ubuntu 16.04 box FWIW.

[SteveF]

I tried building with clang (by changing Makefile.inc to specify clang++ instead of g++) and it failed with a few warnings about unused private members. I've created branch with a few small code changes on to fix these; should we merge this to proposed-updates? You can see the code at https://github.com/stardot/beebasm/tree ... emma/clang

[ctr]

I merged my change. Yours looks good too.

[SteveF]

Cheers, I've merged mine too.

Richard - please do give this a try on whatever OSes you have handy! You should now be able to build with clang without any fuss so no need to install gcc unless you're feeling keen.

[richardtoohey]

Thank you.

OpenBSD, looks good. So this is the proposed-updates branch, no local changes, using gmake (GNU make port, GNU Make 4.2.1) and GCC (version 4.2.1):

Code: Select all

$ gmake all PLATFORM=linux
-e Making subdirectory ... objects/.
-e Generating dependencies for ... random.cpp
-e Generating dependencies for ... expression.cpp
-e Generating dependencies for ... assemble.cpp
-e Generating dependencies for ... asmexception.cpp
-e Generating dependencies for ... discimage.cpp
-e Generating dependencies for ... sourcefile.cpp
-e Generating dependencies for ... stringutils.cpp
-e Generating dependencies for ... lineparser.cpp
-e Generating dependencies for ... globaldata.cpp
-e Generating dependencies for ... macro.cpp
-e Generating dependencies for ... sourcecode.cpp
-e Generating dependencies for ... main.cpp
-e Generating dependencies for ... commands.cpp
-e Generating dependencies for ... objectcode.cpp
-e Generating dependencies for ... symboltable.cpp
-e Generating dependencies for ... BASIC.cpp
-e Compiling ... BASIC.cpp
-e Compiling ... symboltable.cpp
-e Compiling ... objectcode.cpp
-e Compiling ... commands.cpp
-e Compiling ... main.cpp
-e Compiling ... sourcecode.cpp
-e Compiling ... macro.cpp
-e Compiling ... globaldata.cpp
-e Compiling ... lineparser.cpp
-e Compiling ... stringutils.cpp
-e Compiling ... sourcefile.cpp
-e Compiling ... discimage.cpp
-e Compiling ... asmexception.cpp
-e Compiling ... assemble.cpp
-e Compiling ... expression.cpp
-e Compiling ... random.cpp
-e Linking ... ../beebasm
./objects/discimage.o: In function `DiscImage::DiscImage(char const*, char const*)':
discimage.cpp:(.text+0x15ce): warning: strcat() is almost always misused, please use strlcat()
discimage.cpp:(.text+0x15af): warning: strcpy() is almost always misused, please use strlcpy()
./objects/BASIC.o: In function `CopyStringLiteral()':
BASIC.cpp:(.text+0x1e6e): warning: sprintf() is often misused, please use snprintf()
-e Running ... ../beebasm
.xpos
     0000
.ypos
     0001
.colour
     0002
... (lots omitted)
.end
.olddotaddrlo
     1A00
.olddotaddrhi
     1B00
.olddotaddry
     1C00
Saving file 'Code'
$ ../beebasm --help    
beebasm 1.09

FreeBSD 10.4

Fails as expected:

Code: Select all

% gmake -v
GNU Make 4.2.1
Built for amd64-portbld-freebsd10.4
Copyright (C) 1988-2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
webarts@devexess:~/beebasm/beebasm-proposed-updates/src % gmake all PLATFORM=linux
-e Making subdirectory ... objects/.
-e Generating dependencies for ... random.cpp
gmake[1]: g++: Command not found
...
% clang -v
FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
Target: x86_64-unknown-freebsd10.4
Thread model: posix
Selected GCC installation: 

Change the Makefile.inc to use clang/clang++

Code: Select all

CC                              :=              gcc
CXX                             :=              g++
LD                              :=              g++

to

Code: Select all

CC                              :=              clang
CXX                             :=              clang++
LD                              :=              clang++

And try again:

Code: Select all

% gmake all PLATFORM=linux
-e Making subdirectory ... objects/.
-e Generating dependencies for ... random.cpp
-e Generating dependencies for ... expression.cpp
-e Generating dependencies for ... assemble.cpp
-e Generating dependencies for ... asmexception.cpp
-e Generating dependencies for ... discimage.cpp
-e Generating dependencies for ... sourcefile.cpp
-e Generating dependencies for ... stringutils.cpp
-e Generating dependencies for ... lineparser.cpp
-e Generating dependencies for ... globaldata.cpp
-e Generating dependencies for ... macro.cpp
-e Generating dependencies for ... sourcecode.cpp
-e Generating dependencies for ... main.cpp
-e Generating dependencies for ... commands.cpp
-e Generating dependencies for ... objectcode.cpp
-e Generating dependencies for ... symboltable.cpp
-e Generating dependencies for ... BASIC.cpp
-e Compiling ... BASIC.cpp
-e Compiling ... symboltable.cpp
-e Compiling ... objectcode.cpp
-e Compiling ... commands.cpp
-e Compiling ... main.cpp
-e Compiling ... sourcecode.cpp
-e Compiling ... macro.cpp
-e Compiling ... globaldata.cpp
-e Compiling ... lineparser.cpp
-e Compiling ... stringutils.cpp
-e Compiling ... sourcefile.cpp
-e Compiling ... discimage.cpp
-e Compiling ... asmexception.cpp
-e Compiling ... assemble.cpp
-e Compiling ... expression.cpp
-e Compiling ... random.cpp
-e Linking ... ../beebasm
-e Running ... ../beebasm
.xpos
     0000
.ypos
     0001
.colour
     0002
... (lots of output omitted)
.end
.olddotaddrlo
     1A00
.olddotaddrhi
     1B00
.olddotaddry
     1C00
Saving file 'Code'
% ../beebasm --help
beebasm 1.09

FreeBSD 11.1 - basically the same as FreeBSD 10.4 (so not going to repeat the output) but a newer clang version:

Code: Select all

% clang --version
FreeBSD clang version 4.0.0 (tags/RELEASE_400/final 297347) (based on LLVM 4.0.0)
Target: x86_64-unknown-freebsd11.1
Thread model: posix
InstalledDir: /usr/bin

% gmake --version
GNU Make 4.2.1
Built for amd64-portbld-freebsd11.1
Copyright (C) 1988-2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

% pkg info | grep GNU
...
gmake-4.2.1_2                  GNU version of 'make' utility
...

[SteveF]

Thanks for testing that, Richard.

I haven't had those link-time warnings myself. I wonder if they're serious. Probably not, perhaps we should change the code to get rid of them anyway.

[richardtoohey]

The link-time warnings are OpenBSD encouraging the use of safer/less-prone-to-security-issues system calls.

I don't know how portable/available the safer calls are, so using them might lead to breakage elsewhere.

[SteveF]

That's an interesting point. A quick and possibly inconclusive play on my Ubuntu 16.04 box suggests I'd need to install libbsd-dev to build with these functions and I'd probably have to #include <bsd/string.h>. I can imagine this would annoy/put off potential users even on Linux, and I wouldn't be surprised if that function didn't exist (or existed in a different header) on Windows.

I have had a look at rewriting the code to avoid the warnings by using ostringstream, which we already used in other places, so there's no new compatibility problem this way. I've pushed that change up to a new branch in the stardot repo: https://github.com/stardot/beebasm/tree/unsafe-warnings (I intended to do this in my own repo but didn't realise until I'd pushed it; I guess it's not a big deal since it's on its own branch anyway).

I don't think there was anything seriously wrong with the code before this change, although I guess in principle you could get a buffer overflow if you passed a very long argument to -boot. But it seems a good idea to avoid generating warnings on any platform if we can help it.

[crj]

The link-time warnings are OpenBSD encouraging the use of safer/less-prone-to-security-issues system calls.

I don't know how portable/available the safer calls are, so using them might lead to breakage elsewhere.

Under Linux, you should be using strncat() and strncpy().

Alternatively, if you care about simplicity more than performance (and in an assembler that won't often be generating more than 32Kbytes of output, I suspect that will probably be the case!), consider strdup() and asprintf().

[ctr]

I have had a look at rewriting the code to avoid the warnings by using ostringstream, which we already used in other places, so there's no new compatibility problem this way.

This builds fine on Windows. The boot functionality works. I also tested a couple of the BASIC error messages and the rest look OK. Being super picky, discimage.cpp lines 135-137 are indented with spaces rather than tabs, so they are much more indented in visual studio than everything else.

[SteveF]

Thanks for being picky, I missed those. I really need to set up my vim to automatically use the right conventions when working on beebasm. I've pushed a new commit to the branch with that fixed.

(I suspect I may have already introduced some inconsistency in space/tab/indent stuff myself in earlier commits, and it may or may not have been consistent to start with. But I haven't checked, and I'm not going to start altering whitespace all over the place now, it will just make merging horrible. If it is inconsistent, perhaps we can fix it early after 1.09 is released. But I'm happy to fix this one, as it's a new inconsistency.)

I'll merge this to proposed-updates in the next day or two unless anyone objects first.