Fix SLANG_ENABLE_ASAN CMake option
#8980
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,16 +69,30 @@ SlangResult CommandLineDownstreamCompiler::compile( | |
| if (!isVersionCompatible(inOptions)) | ||
| { | ||
| // Not possible to compile with this version of the interface. | ||
| // TODO: Fix other `IDownstreamCompiler::compile` implementations not always writing to | ||
| // `outArtifact` when returning early. | ||
| const auto targetDesc = ArtifactDescUtil::makeDescForCompileTarget(SLANG_TARGET_UNKNOWN); | ||
| auto artifact = ArtifactUtil::createArtifact(targetDesc); | ||
| *outArtifact = artifact.detach(); | ||
| return SLANG_E_NOT_IMPLEMENTED; | ||
| } | ||
|
|
||
| CompileOptions options = getCompatibleVersion(&inOptions); | ||
| const auto targetDesc = ArtifactDescUtil::makeDescForCompileTarget(options.targetType); | ||
|
|
||
| // Create the result artifact | ||
| auto resultArtifact = ArtifactUtil::createArtifact(targetDesc); | ||
|
|
||
| // Create the diagnostics | ||
| auto diagnostics = ArtifactDiagnostics::create(); | ||
| ArtifactUtil::addAssociated(resultArtifact, diagnostics); | ||
|
|
||
| // If Slang was built with Clang or GCC and sanitizers were enabled, the same `-fsanitize=...` | ||
| // flag must be passed when linking an executable or shared library with a Slang library, or | ||
| // linking will fail. We can't instrument C++ generated from Slang code with sanitizers as they | ||
| // might report errors that we can't fix, so we separate compilation and linking into two | ||
| // commands to enable sanitizers only during linking. | ||
| bool shouldSeparateCompileAndLink = options.targetType != SLANG_OBJECT_CODE; | ||
|
|
||
| auto helper = DefaultArtifactHelper::getSingleton(); | ||
|
|
||
|
|
@@ -95,8 +109,13 @@ SlangResult CommandLineDownstreamCompiler::compile( | |
| { | ||
| // We could use the path to the source, or use the source name/paths as defined on the | ||
| // artifact For now we just go with a lock file based on "slang-generated". | ||
| if (SLANG_FAILED(helper->createLockFile( | ||
| asCharSlice(toSlice("slang-generated")), | ||
| lockFile.writeRef()))) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
|
Contributor
There was a problem hiding this comment. I don't know this part of slangc very well (yet), but I'd just like to double-check that this is intentional. Usually, a failure should rollback whatever changes was done by the function. In this case, it feels a bit odd that we're returning a failure but still also returning the artifact. See also https://en.wikipedia.org/wiki/Exception_safety Addendum. I see from above that apparently this is the intention. But I do wonder if the call site shouldn't try to use the artifact in case this function failed, unless it's for some sort of diagnostics (= error output). I this case, I would appreciate a code comment. |
||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
| auto lockArtifact = Artifact::create( | ||
| ArtifactDesc::make(ArtifactKind::Base, ArtifactPayload::Lock, ArtifactStyle::None)); | ||
|
|
@@ -110,17 +129,81 @@ SlangResult CommandLineDownstreamCompiler::compile( | |
| options.modulePath = SliceUtil::asTerminatedCharSlice(modulePath); | ||
| } | ||
|
|
||
| // Compile stage: compile source to object code | ||
|
Contributor
There was a problem hiding this comment. Why do we need this newly added code just as an ASAN fix? (I presume we do. The PR description would be the place to explain why. Not at all obvious.) |
||
| ComPtr<IArtifact> objectArtifact; | ||
|
|
||
| if (shouldSeparateCompileAndLink) | ||
| { | ||
| CompileOptions compileOptions = options; | ||
| compileOptions.targetType = SLANG_OBJECT_CODE; | ||
|
|
||
| CommandLine compileCmdLine(m_cmdLine); | ||
| if (SLANG_FAILED(calcArgs(compileOptions, compileCmdLine))) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
| List<ComPtr<IArtifact>> compileArtifacts; | ||
| if (SLANG_FAILED(calcCompileProducts( | ||
| compileOptions, | ||
| DownstreamProductFlag::All, | ||
| lockFile, | ||
| compileArtifacts))) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
| // There should only be one object file (.o/.obj) as artifact | ||
| SLANG_ASSERT(compileArtifacts.getCount() == 1); | ||
| SLANG_ASSERT(compileArtifacts[0]->getDesc().kind == ArtifactKind::ObjectCode); | ||
| objectArtifact = compileArtifacts[0]; | ||
|
|
||
| ExecuteResult compileResult; | ||
| if (SLANG_FAILED(ProcessUtil::execute(compileCmdLine, compileResult)) || | ||
| SLANG_FAILED(parseOutput(compileResult, diagnostics))) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
| // If compilation failed, return the diagnostics | ||
| if (compileResult.resultCode != 0 || !objectArtifact->exists()) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
| } | ||
|
|
||
| // Link stage (or single-stage compile for object code targets) | ||
| CommandLine cmdLine(m_cmdLine); | ||
|
|
||
| if (shouldSeparateCompileAndLink) | ||
| { | ||
| // Pass compiled object to linker | ||
| options.sourceArtifacts = makeSlice(objectArtifact.readRef(), 1); | ||
| } | ||
|
|
||
| // Append command line args to the end of cmdLine using the target specific function for the | ||
| // specified options | ||
| if (SLANG_FAILED(calcArgs(options, cmdLine))) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
| // The 'productArtifact' is the main product produced from the compilation - the | ||
| // executable/sharedlibrary/object etc | ||
| ComPtr<IArtifact> productArtifact; | ||
| { | ||
| List<ComPtr<IArtifact>> artifacts; | ||
| if (SLANG_FAILED( | ||
| calcCompileProducts(options, DownstreamProductFlag::All, lockFile, artifacts))) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
| for (IArtifact* artifact : artifacts) | ||
| { | ||
|
|
@@ -139,6 +222,7 @@ SlangResult CommandLineDownstreamCompiler::compile( | |
| // Somethings gone wrong if we don't find the main artifact | ||
| if (!productArtifact) | ||
| { | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
|
|
@@ -152,7 +236,13 @@ SlangResult CommandLineDownstreamCompiler::compile( | |
| } | ||
| #endif | ||
|
|
||
| if (SLANG_FAILED(ProcessUtil::execute(cmdLine, exeRes)) || | ||
| SLANG_FAILED(parseOutput(exeRes, diagnostics))) | ||
| { | ||
| // If the process failed or the output parsing failed, return the diagnostics | ||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_FAIL; | ||
| } | ||
|
|
||
| #if 0 | ||
| { | ||
|
|
@@ -209,23 +299,13 @@ SlangResult CommandLineDownstreamCompiler::compile( | |
| } | ||
| } | ||
|
|
||
| // Find the rep from the 'main' artifact, we'll just use the same representation on the output | ||
| // artifact. Sharing is desirable, because the rep owns the file. | ||
| if (auto fileRep = productArtifact | ||
| ? findRepresentation<IOSFileArtifactRepresentation>(productArtifact) | ||
| : nullptr) | ||
| { | ||
| resultArtifact->addRepresentation(fileRep); | ||
| } | ||
|
|
||
| // Add the artifact list if there is anything in it | ||
|
|
@@ -242,10 +322,10 @@ SlangResult CommandLineDownstreamCompiler::compile( | |
|
|
||
| artifactContainer->setChildren(slice.data, slice.count); | ||
|
|
||
| resultArtifact->addAssociated(artifactContainer); | ||
| } | ||
|
|
||
| *outArtifact = resultArtifact.detach(); | ||
| return SLANG_OK; | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes such as this should be commented in the PR description. It is not obvious to the reviewer why you removed this flag. Presumably there was a good reason.