Emulator reset is broken

[daid]

Not sure what all was changed around this, but emulator reset no longer works properly.

If you run this example:
https://raw.githubusercontent.com/daid/gameboy-assembly-by-example/master/graphics/sprites.asm

The first run is fine, and the 2nd run after a reset has corrupted sprites. Indicating that something went wrong with VRAM access, which is odd that it only happens the 2nd time.

[SelvinPL]

it's problem with low part of rom
add

SECTION "blank", ROM0[$0]
  jp start
  ds $10-@, 0  

[ISSOtm]

Wait, that would mean the emulator starts at PC=0 when reset, instead of PC=$100? That's a problem.

[SelvinPL]

it's seems to be problem with binjgb - what i belive it happens that emulator "hides" lower addresses to long (if I understand it correctly on real hardware program starts from address 0 but not from rom but som internal bootrom)

[ISSOtm]

Yes, the console starts at 0 and runs a boot ROM that yields back control at $100, but that can be mostly papered over as if the console started at $100 in a post-boot ROM state directly.

The bug would be that the emulator starts at $100 the first time, but then at 0 when reset?

[SelvinPL]

hmm it is not bootrom ... this emulator doesn't even have bootrom ....
this is offensive line

wrapper.c: emulator_new_simple -> lets observe rom_data passes from javascript
emulator.c: emulator_new -> rom_data is passed as init.rom.data (which is pointer)
set_rom_file_data -> init.rom is set to e->file_data

then when you call emulator_delete it calls file_data_delete(&e->file_data)
and it calls xfree(file_data->data)

and if messing with input buffer which should not be freed but binjgb library

I did comment this line and now this is fixed .. but do not know if file_data_delete is called in other case and then it would cause memory leaks

[SelvinPL]

eventually patching this file (common.c in binjgb library) to delete offensive line could help

[SelvinPL]

it's problem with low part of rom add

SECTION "blank", ROM0[$0]
  jp start
  ds $10-@, 0  

hehe, before I posted this I was testing something and put jp start which is irrevelant here
The point was that you need to fill at least $8-$10 bytes of rom to linker not put the stuff there

in OP's example SECTION "graphics", ROM0 goes to address 0 and it is get frack by xfree error

[SelvinPL]

hmmm second option is to remove
free call from javascript code here

[ISSOtm]

Er, that sounds like a much deeper issue. What I find puzzling is that binjgb makes a copy of the ROM on load, so there should only be a single pointer to that ROM. (Put differently, we should always be freeing the ROM after calling emulator_new_simple.)

[SelvinPL]

this is not a copy but pointer assigment

[ISSOtm]

No:

static Result set_rom_file_data(Emulator* e, const FileData* file_data) {
  CHECK_MSG(file_data->size > 0, "File is empty.\n");
  CHECK_MSG((file_data->size & (MINIMUM_ROM_SIZE - 1)) == 0,
            "File size (%ld) should be a multiple of minimum rom size (%ld).\n",
            (long)file_data->size, (long)MINIMUM_ROM_SIZE);
  e->file_data = *file_data;
  return OK;
  ON_ERROR_RETURN;
}

* in *file_data does dereference the pointer.

[SelvinPL]

sure ... but structure holds pointer

typedef struct FileData {
  u8* data;
  size_t size;
} FileData;

so xfree(file_data->data) fracks this

[ISSOtm]

Nope, it holds a FileData.

Oh, you were talking about FileData itself!

Yes, indeed, it does take ownership of the ROM... at least on success. Good catch! We were double-freeing this.

[SelvinPL]

I think this is solution #28 (comment)

edit: frack this #28 (comment)

[ISSOtm]

Yes, that would be correct. (I would add a comment right above emulator_new_simple that it takes ownership of the allocation, and will free it on its own when destroyed.)

[SelvinPL]

Yes, that would be correct. (I would add a comment right above emulator_new_simple that it takes ownership of the allocation, and will free it on its own when destroyed.)

yeah, one year from now someone would add free again :D

[ISSOtm]

I cannot reproduce the error in the OP anymore. Why was the issue reopened?

[daid]

Looks fixed for this specific case, didn't check if is luck or if the timing is 100% the same now for both runs.