Add a target option for simc - #364
Conversation
cc8d942 to
416a8df
Compare
|
needs rebase |
|
416a8df needs rebase |
416a8df to
5d57119
Compare
|
Rebased |
5d57119 to
07ce8ba
Compare
|
Made some improvements per @KyrylR's suggestions |
07ce8ba to
235a814
Compare
|
needs rebase |
235a814 to
ad3a9e8
Compare
ad3a9e8 to
c0772e0
Compare
| #[cfg(feature = "external-jets")] | ||
| Target::External(path) => { | ||
| unsafe { | ||
| init_external_jet_lib(path)?; |
There was a problem hiding this comment.
In c0772e0:
This unsafe code needs a SAFETY: comment explaining why it's sound.
Also, this unsafe code is not sound. You need to do some sort of validation on path before you can call init_external_jet_lib.
There was a problem hiding this comment.
I pushed changes to resolve this
There was a problem hiding this comment.
The user MUST trust the library to implement the external jet ABI. is not a reasonable safety comment. We cannot ship code that exhibits UB based on user-provided input.
There was a problem hiding this comment.
I suppose, I've written this in the wrong way. What I meant with this, that the dynamic library carries arbitrary binary code and we are not responsible for what it will decide to execute.
I do say "Initialization verifies that every required symbol exists" a line below and it actually does, so it should eliminate UB.
Maybe it would be better to just remove this:
"The user MUST trust the library to implement the external jet ABI."
As we do check that ABI is present.
There was a problem hiding this comment.
In 7172f08:
I think we should move validate_external_jet_lib_path to right before the unsafe call. As written, Target has a public Target::External(string) constructor, but the SAFETY comment claims that the string has been validated. We can't enforce that without changing the enum or moving the check.
I also wonder whether we need to use String or str here at all, since it imposes a UTF-8 requirement on paths that we shouldn't need to.
7172f08 to
667d0b3
Compare
stringhandler
left a comment
stringhandler
left a comment
There was a problem hiding this comment.
I think the feature gating is ok, but I would say that we should always print the options in clap, but rather error if they are used without the correct compiled features.
Some other LLM findings:
Output records compiler_version but not the target, and the target changes the CMR
The doc comment on compiler_version at line 30 gives the rationale for carrying it: "Different compiler versions can produce different CMRs from the same source, so the version travels with the artifact as metadata."
That reasoning now applies verbatim to --target. Same source, same compiler, different target:
$ simc examples/cat.simf # CMR e65e19e139a13583a0a7efb24be13c20d578f06f51b2a7fe7c7b9097072dbabe
$ simc -t core examples/cat.simf # CMR c83aea0e102548c2a441c6d4d268fb0469263601ef705ea61fa18882938ec06f
With --json consumers now unable to tell which jet set an artifact was built against, a target: String field on Output seems like it belongs in this PR. (It would need Display/Debug on Target, which it doesn't currently derive.)
| "core" => Ok(Target::Core), | ||
| #[cfg(feature = "external-jets")] | ||
| _ if s.starts_with("external:") => { | ||
| let path = s.trim_start_matches("external:"); |
There was a problem hiding this comment.
in 667d0b3
strip_prefix is more suitable than trim_start_matches here. trim_start_matches removes multiple occurrences
| #[derive(Debug)] | ||
| struct TargetParseError(String); | ||
|
|
||
| impl fmt::Display for TargetParseError { |
There was a problem hiding this comment.
LLM finding:
The Display impl is never reached — errors print via Debug
main returns Result<(), Box>, and Rust's Termination impl formats the error with Debug, not Display. Because TargetParseError uses #[derive(Debug)], the hand-written Display at line 62 is dead for every error that escapes jet_hinter(), and the user sees the derived tuple-struct form:
$ simc --target external:./foo.dylib examples/cat.simf
Error: TargetParseError("Invalid external target path './foo.dylib': No such file or directory (os error 2)")
Implementing Debug by hand to delegate to Display fixes it in one place:
impl fmt::Debug for TargetParseError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(self, f)
}
}
Related: because path validation lives in jet_hinter() rather than from_str, the two failure modes for the same flag are formatted completely differently — external: (empty) gets clap's error: invalid value 'external:' for '--target ': …, while external:./missing.dylib gets the raw Error: TargetParseError(…) above. Moving validate_external_jet_lib_path into from_str would make both clap-formatted and would fail fast before any source file is read. The dlopen itself would stay in jet_hinter().
| ))); | ||
| } | ||
|
|
||
| let expected_extension = std::env::consts::DLL_EXTENSION; |
There was a problem hiding this comment.
LLM finding:
The extension check rejects loadable libraries
std::env::consts::DLL_EXTENSION is a single value per platform (dylib on macOS, so on Linux, dll on Windows), but dlopen accepts more than that:
macOS loads .so files fine — this is exactly what you get from a library built on Linux, or from build systems that emit .so everywhere. Currently rejected:
$ simc --target external:/tmp/lib.so examples/cat.simf
Error: … expected a .dylib shared library
Versioned Linux sonames are rejected — Path::extension() on libfoo.so.1 returns "1", so the canonical libfoo.so.1 form fails on the platform where it's most common.
Since the real safety gate is Library::load (which fails loudly on a non-library) plus the symbol resolution in ExternalJetLib::load, this check buys little and costs valid inputs. I'd either accept the set {so, dylib, dll} regardless of host, or drop the check and let the loader produce the error.
667d0b3 to
fc4f030
Compare
|
I iterated on all comments that been written here |
4 participants
This PR creates a new compilation option to choose a target jet set. Closes #224