AppleWin / AppleWin

Apple II emulator for Windows
GNU General Public License v2.0
703 stars 162 forks source link

(Debugger) Duplicate symbols for identical memory addresses in APPLE2E.SYM #244

Open TommyGH opened 9 years ago

TommyGH commented 9 years ago

Start AppleWin and enter debugger (click on magnifying glass)

sym Main:760 .... SYMMAIN LOAD Main:767 sym Main:760 ...

I was confused about the difference from 760 to 767 and then back to 760.

Not sure if a correction is needed, but I found the following 7 duplicates in APPLE2E.SYM:

0478 A2C.ROMSTATE 0478 TEXT.HOLE.1

04F8 A2C.TEMP1 04F8 TEXT.HOLE.2

04FB A2C.VMODE 04FB DOS33.MODE

0578 A2C.TEMPA 0578 TEXT.HOLE.3

0778 A2C.DEVN0 0778 TEXT.HOLE.7

07F8 MSLOT 07F8 TEXT.HOLE.8

C08E SETREADMODE C08E WRITEPROT

Maybe a warning message is needed to indicate the duplicate entry will overwrite the previously defined entry.

Tommy

TommyGH commented 9 years ago

I used the greater than symbol (before "sym" and "symmain load" in my description above. GITHUB doesn't just echo them to the screen, but interprets them somehow?

Michaelangel007 commented 9 years ago

It is not a bug to have duplicate symbols.

Debugger_Symbols.cpp ParseSymbolTable() actually prints duplicate symbols to the debugger console. (I need to check which debugger version is enabled in.)

TommyGH commented 9 years ago

I realize this issue is minor, but I would like to add just a few more words (well maybe a lot).

When I worked in testing and software support, we had a philosophy that anything that might confuse the average user is likely to be a bug (meaning something needs to be fixed, either software, documentation, etc.)

The bug here is being confused by the displayed numbers in the following sequence in which no warnings or error messages are displayed, nor are any duplicate entries displayed:

  1. Create A2_USER1.SYM with: 0000 Tom1 0000 Tom2
  2. SYMUSER CLEAR Cleared symbol table: User1
  3. SYM Main:760 Basic:635 Asm:0 User1:0
  4. SYMUSER LOAD User1:2
  5. SYM Main:760 Basic:635 Asm:0 User1:1

Notice that step 4 showed "2", but now at step 5 it magically shows "1". This was confusing to me and I would contend to the average user who should spend some time trying to figure out why.

If you say that duplicate labels in a SYM file are ok, then what happens when the user removes the second occurrence using the SYMUSER command? Does the first label now work?

SYMUSER ! Tom2

Obviously I know the answer, but the point is that the first label just isn't defined. The user will never see it unless s/he looks in the file.

Now as you pointed out, sometimes the user DOES get an error message about "Dup Symbol Name" in the SYM file. This isn't one of them. To get an error message some times and not others also leads to confusion.

Add to A2_USER1.SYM the following: c000 keyboard

  1. SYMMAIN ON
  2. SYMUSER CLEAR
  3. SYMUSER LOAD

This displays the "Dup Symbol Name" error for "$C000" but does not display anything about "Tom1" or "Tom2".

Now the argument could be made that it would be foolish to have "holes" in some of the descriptions in the SYM file. For example:

0000 zero 0001 one 0002 two 0003 three 0001 tom

It might be confusing to just remove "0001 one" and leave a gap there. So what to do? Well, it looks like in the past that those definitions that are never seen would be turned into comments by placing a semicolon in column 1.

0000 zero ;0001 one 0002 two 0003 three 0001 tom

And now I put on my software developer's hat and say:

Really!? Nothing better to do on a Saturday night than nitpick a minor bug?

I do understand that in software development that there is no such thing as a "simple" software change. There is effort in documenting the change, getting the change integrated, etc. On top of that, this is one of those changes that can cause headaches in the future because now there will be at least 2 versions of almost identical files roaming around in the wild.

If you think someone in the future might be confused by the sequence that I have shown, then perhaps a fix is warranted. Even then you might want to weigh that versus the effort to make any changes at all.

Tommy

Michaelangel007 commented 9 years ago

This is a display bug -- what is happening is that we read nDisplaySize symbols, but the .size() is the total amount without duplicates.

void _CmdSymbolsInfoHeader( int iTable, char * pText, int nDisplaySize /* = 0 */ )
{
    bool bActive = (g_bDisplaySymbolTables & (1 << iTable)) ? true : false;
    int nSymbols  = nDisplaySize ? nDisplaySize : g_aSymbols[ iTable ].size();

The more important question is why did CmdSymbolsLoad() not report duplicates?!

Logic bug? in ParseSymbolTable() -- only checking for duplicate names, not duplicate addresses!

bool bExists = FindAddressFromSymbol( sName, &nAddressPrev, &iTable );
Michaelangel007 commented 9 years ago

Here is a preview of Debugger 2.8.0.5

debug_symbols_warning

Michaelangel007 commented 9 years ago

This bug can't be closed until we fix the display of:

a) total symbols read, AND b) number symbols actually added

to prevent confusion.

ParseSymbolTable()
    nSymbolsLoaded --> nSymbolsRead
Michaelangel007 commented 9 years ago

Tommy, here is Debugger 2.8.0.6 nightly build that also has Bug #239 working: http://michael.peopleofhonoronly.com/dev/applewin/nightlybuild/Applewin.exe

e.g.

 Warning: HIGHDSL          aliases $0094 HIGHDS        (Basic)                  
C:\Dev\AppleWin.Git\Release\A2_USER1.SYM                                        
 Warning: Tom1             aliases $0000 GOWARM        (Basic)                  
 Warning: Tom2             aliases $0000 Tom1          (User1)