Skip to content

Import memory caused to Fatal: Module::addMemory: 0 already exists #4988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGraey opened this issue Aug 29, 2022 · 2 comments · Fixed by #4991
Closed

Import memory caused to Fatal: Module::addMemory: 0 already exists #4988

MaxGraey opened this issue Aug 29, 2022 · 2 comments · Fixed by #4991

Comments

@MaxGraey
Copy link
Contributor

After #4811 we find out that BinaryenSetMemory + BinaryenAddMemoryImport produces fatal error:

Fatal: Module::addMemory: 0 already exists
@MaxGraey
Copy link
Contributor Author

MaxGraey commented Aug 29, 2022

Currently BinaryenSetMemory and BinaryenAddMemoryImport both create new memory and add it via ((Module*)module)->addMemory internally. I propose change BinaryenAddMemoryImport only to modify memory->module and memory->base like:

// Or create new API - BinaryenSetMemoryImport
void BinaryenAddMemoryImport(BinaryenModuleRef module,
                             const char* name,
                             const char* externalModuleName,
                             const char* externalBaseName,
                             /* unused */ uint8_t shared) {
  auto* memory = ((Module*)module)->getMemoryOrNull(name);
  if (memory == nullptr) {
    Fatal() << "invalid memory '" << name 
            << "'. Make sure BinaryenSetMemory was called before BinaryenAddMemoryImport";
  }
  memory->module = externalModuleName;
  memory->base = externalBaseName;
}

instead:

void BinaryenAddMemoryImport(BinaryenModuleRef module,
                             const char* internalName,
                             const char* externalModuleName,
                             const char* externalBaseName,
                             uint8_t shared) {
  auto memory = Builder::makeMemory(internalName);
  memory->module = externalModuleName;
  memory->base = externalBaseName;
  memory->shared = shared;
  ((Module*)module)->addMemory(std::move(memory));
}

WDYT?

@kripken
Copy link
Member

kripken commented Aug 30, 2022

Sounds reasonable. But we can make it backwards compatible too, that is, by gracefully handling the case where the memory already exists. I think that would be best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants