Allow ChiselSim to link in prebuilt software libraries#5189
Allow ChiselSim to link in prebuilt software libraries#5189fabianschuiki merged 1 commit intomainfrom
Conversation
|
Not quite sure how to deal with the binary compatibility stuff 🤔 |
| entry.split("=", 2) match { | ||
| case Array(name, path) => name -> path | ||
| case _ => | ||
| throw new IllegalArgumentException( | ||
| s"Invalid link library mapping `$entry`; expected `<name>=<path>`" | ||
| ) |
There was a problem hiding this comment.
This is fine but bonus points if you give an error with the bad input and a carat, e.g.
chisel/src/main/scala/chisel3/stage/ChiselAnnotations.scala
Lines 97 to 111 in fad1f6c
| val libraryMapFromProp = parseLibraryMap(sys.props.get("chiselsim.libraries")) | ||
| val libraryMapFromEnv = parseLibraryMap(sys.env.get("CHISELSIM_LIBS")) |
There was a problem hiding this comment.
Usually these things are either or (and also either-or with being passed as arguments).
For example, you can set the CLASSPATH environment variable, but if you set it via -cp <classpath> that wins and the environment variable is ignored.
I'm not sure if we should do that here, but maybe? What do you think?
There was a problem hiding this comment.
I didn't know about the classpath behavior. My gut feeling is that since these are lists, we might as well combine them instead of having a single library provided via one mechanism effectively remove the ones provided in the other. That feels somewhat unexpected from a user point of view.
There was a problem hiding this comment.
My intuition remains that overriding is more likely to be expected by users, but I'm also not going to sandbag on that because I agree that concatenation also feels reasonable.
| val libraryMapFromEnv = parseLibraryMap(sys.env.get("CHISELSIM_LIBS")) | ||
| val libraryMap = libraryMapFromProp ++ libraryMapFromEnv | ||
| val resolvedLibraryPaths = settings.libraries.map { name => | ||
| libraryMap.get(name) match { |
There was a problem hiding this comment.
I'm a little confused, so the libraries arguments are checked against the library mappings from the environment/system properties. What are the libraryPath arguments then?
There was a problem hiding this comment.
They are a mechanism to provide a path in the Scala code directly. You'll likely not use that in practice, but there's no reason to not have that as a fallback option in case people work in some kind of fixed environment, or for testing purposes.
| // Compile shared libraries for the simulator link library tests. | ||
| def sharedTestLibs: T[Seq[os.Path]] = Task(persistent = true) { | ||
| val srcDir = BuildCtx.workspaceRoot / "src" / "test" / "resources" / "chisel3" / "simulator" | ||
| val cc = sys.env.getOrElse("CC", "cc") | ||
| val shared = if (scala.util.Properties.isMac) "-dynamiclib" else "-shared" | ||
| val srcFiles = Seq("linkLibA.c", "linkLibB.c", "linkLibC.c") | ||
| val libFiles = srcFiles.map { src => | ||
| val lib = Task.dest / (src.stripSuffix(".c") + ".so") | ||
| os.proc(cc, shared, "-fPIC", srcDir / src, "-o", lib).call() | ||
| lib | ||
| } | ||
| libFiles | ||
| } | ||
|
|
||
| // Pass some of the pre-compiled shared libraries to the simulator via | ||
| // environment variables. | ||
| override def forkEnv = Task { | ||
| val libs = sharedTestLibs() | ||
| super.forkEnv() ++ Map( | ||
| // CHISELSIM_LIBS lets ChiselSim resolve libraries by name: | ||
| "CHISELSIM_LIBS" -> s"linkLibA=${libs(0)}", | ||
| // Also pass in a full path to test the `libraryPaths` setting: | ||
| "LINKLIBC_FULL_PATH" -> libs(2).toString | ||
| ) | ||
| } | ||
|
|
||
| // Pass one of the pre-compiled shared libraries to the simulator via a JVM | ||
| // property. | ||
| override def forkArgs = Task { | ||
| val libs = sharedTestLibs() | ||
| super.forkArgs() :+ s"-Dchiselsim.libraries=linkLibB=${libs(1)}" | ||
| } |
There was a problem hiding this comment.
I'm wondering if instead of applying this to all chisel tests, we shouldn't add a new test build unit for this. Obviously they aren't harmful, but it feels a bit incongruous
There was a problem hiding this comment.
Yeah I was wondering the same thing. I then thought that separating this out into a different build would complicate the whole build for no good practical reason, just aesthetics.
fabianschuiki
force-pushed
the
fschuiki/chiselsim-libs
branch
from
bff3724 to
c3a7b43
Compare
fabianschuiki
force-pushed
the
fschuiki/chiselsim-libs
branch
2 times, most recently
from
547e3e2 to
789f854
Compare
Add the `libraries` and `libraryPaths` settings to ChiselSim's `Settings` class that can be passed to `simulate(...)`. These settings allow the user to provide a list of library names or paths that the simulator backend should link into the simulator. I envision this to be used for DPI-based tests to link in implementations for DPI functions. The build system can use the `CHISELSIM_LIBS` environment variable or the `chiselsim.libraries` Java property to provide a list of library names and their paths which the user can call out in `libraries`. This allows the Chisel code to not hardcode filesystem paths that are likely to be a build system concern, and instead only call out libraries by name. This commit also adds a small C compiler invocation to `build.mill` in order to build a handful of shared libraries that the simulator unit tests will use to check the library linking behavior.
fabianschuiki
force-pushed
the
fschuiki/chiselsim-libs
branch
from
789f854 to
cb0413e
Compare
jackkoenig
left a comment
jackkoenig
left a comment
There was a problem hiding this comment.
Some nits but I'm okay with merging
| simulationSettings: CommonSimulationSettings = CommonSimulationSettings.default | ||
| simulationSettings: CommonSimulationSettings = CommonSimulationSettings.default, | ||
| // @unroll is required to maintain binary compatibility | ||
| @unroll linkLibraryPaths: Seq[String] = Seq.empty |
There was a problem hiding this comment.
This is the greatest discovery of my life.
| override def forkEnv = Task { | ||
| val libs = sharedTestLibs() | ||
| super.forkEnv() ++ Map( | ||
| // CHISELSIM_LIBS lets ChiselSim resolve libraries by name: | ||
| "CHISELSIM_LIBS" -> s"linkLibA=${libs(0)}", | ||
| // Also pass in a full path to test the `libraryPaths` setting: | ||
| "LINKLIBC_FULL_PATH" -> libs(2).toString | ||
| ) | ||
| } | ||
|
|
||
| // Pass one of the pre-compiled shared libraries to the simulator via a JVM | ||
| // property. | ||
| override def forkArgs = Task { | ||
| val libs = sharedTestLibs() | ||
| super.forkArgs() :+ s"-Dchiselsim.libraries=linkLibB=${libs(1)}" | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not going to block but I don't love this meaning given to the various libs by index--I'd rather we used a case class (or even named tuple, <3 Scala 3) to give names to them, but I'll let it go.
2 participants
Add the
librariesandlibraryPathssettings to ChiselSim'sSettingsclass that can be passed tosimulate(...). These settings allow the user to provide a list of library names or paths that the simulator backend should link into the simulator. I envision this to be used for DPI-based tests to link in implementations for DPI functions. The build system can use theCHISELSIM_LIBSenvironment variable or thechiselsim.librariesJava property to provide a list of library names and their paths which the user can call out inlibraries. This allows the Chisel code to not hardcode filesystem paths that are likely to be a build system concern, and instead only call out libraries by name.This commit also adds a small C compiler invocation to
build.millin order to build a handful of shared libraries that the simulator unit tests will use to check the library linking behavior.Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash)and clean up the commit message.Create a merge commit.