Skip to content
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

feat(c-api) Implement cross-compilation API in C #2072

Merged
merged 17 commits into from
Feb 2, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jan 28, 2021

Description

Depends on #2071 (merged).

This patch implements wasm_target_t, wasm_triple_t, wasm_cpu_features_t.

This patch also implements wasm_config_set_target to set… a wasm_target_t. The wasm_config_new_with_engine function has been updated to handle the new wasm_target_t.

Example

int main() {
    // Create the configuration.
    wasm_config_t* config = wasm_config_new();

    // Set the target.
    {
      // Declare the target triple.
      wasm_triple_t* triple;
  
      {
          wasm_name_t triple_name;
          wasm_name_new_from_string(&triple_name, "x86_64-apple-darwin");
  
          triple = wasm_triple_new(&triple_name);
  
          wasm_name_delete(&triple_name);
      }
  
      assert(triple);
  
      // Declare the target CPU features.
      wasm_cpu_features_t* cpu_features = wasm_cpu_features_new();
  
      {
          wasm_name_t cpu_feature_name;
          wasm_name_new_from_string(&cpu_feature_name, "sse2");
  
          wasm_cpu_features_add(cpu_features, &cpu_feature_name);
  
          wasm_name_delete(&cpu_feature_name);
      }
  
      assert(cpu_features);
  
      // Create the target!
      wasm_target_t* target = wasm_target_new(triple, cpu_features);
      assert(target);
  
      // Set the target onto the configuration!
      wasm_config_set_target(config, target);
    }

    // Create the engine.
    wasm_engine_t* engine = wasm_engine_new_with_config(config);

    // Check we have an engine!
    assert(engine);

    // Free everything.
    wasm_engine_delete(engine);

    return 0;
}

Review

  • Add a short description of the the change to the CHANGELOG.md file

@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests 📚 documentation Do you like to read? labels Jan 28, 2021
@Hywan Hywan requested a review from MarkMcCaskey January 28, 2021 16:29
@Hywan Hywan self-assigned this Jan 28, 2021
@Hywan Hywan changed the title Feat c api cross compilation 2 feat(c-api) Implement cross-compilation API in C Jan 28, 2021
@Hywan Hywan force-pushed the feat-c-api-cross-compilation-2 branch from 0fa667a to 169fe16 Compare January 29, 2021 11:34
@Hywan Hywan marked this pull request as ready for review January 29, 2021 13:35
@Hywan Hywan requested a review from jubianchi as a code owner January 29, 2021 13:35
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good overall

let mut builder = ObjectFile::headless();

if let Some(target) = config.target {
builder = builder.target(target.inner);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to set the target for a headless-only engine?

It's fine for symmetry I suppose but I don't think this can ever do anything

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was wondering if that was making sense. Using the target makes sense only when compiling, but we never know, maybe one day, someone, somewhere… :-p. Let's keep it for “the symmetry” as you said, how does it sound?

builder = builder.target(target.inner);
}

Arc::new(builder.engine())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ones too

lib/c-api/src/wasm_c_api/unstable/target_lexicon.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/unstable/target_lexicon.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/unstable/target_lexicon.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/unstable/target_lexicon.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/unstable/target_lexicon.rs Outdated Show resolved Hide resolved
lib/c-api/src/wasm_c_api/unstable/target_lexicon.rs Outdated Show resolved Hide resolved
Comment on lines +292 to +293
cpu_features: Option<&mut wasm_cpu_features_t>,
feature: Option<&wasm_name_t>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need Option around all user-supplied pointers, the standard wasm-c-api doesn't bother with that in a lot of cases, if the pointer is invalid it'll crash or do what C does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but I prefer to prevent bad usage of our API. That's what we already do everywhere almost.

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

bors r+

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

bors r-

waiting on #2083 to finish

@bors
Copy link
Contributor

bors bot commented Feb 2, 2021

Canceled.

@Hywan
Copy link
Contributor Author

Hywan commented Feb 2, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 2, 2021

@bors bors bot merged commit 63acc46 into wasmerio:master Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Do you like to read? 🎉 enhancement New feature! 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants