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

Reflection compiler option #5507

Merged
merged 12 commits into from
Nov 11, 2024

Conversation

tareksander
Copy link
Contributor

Fixes #5492

Some questions:

  • Is OSFilesystem the correct API to write a file?
  • Does saveFile expect a NULL-terminated string? I kind of just expected it to, since there is no length argument.

All the reflection tests still seemed to pass.

@tareksander tareksander requested a review from a team as a code owner November 6, 2024 21:18
Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

Thank you for putting these together! It all looks good to me. Only minor comments.

Would also be good if we can also get this added to our COM-based compilation API, in the form of IComponentType::getReflectionJson(IBlob** outBlob).

reflectionPath.appendChar('\0');
auto bufferWriter = PrettyWriter();
emitReflectionJSON(this, this->getReflection(), bufferWriter);
if (SLANG_FAILED(OSFileSystem::getMutableSingleton()->saveFile(reflectionPath.getBuffer(), bufferWriter.getBuilder().getBuffer(), bufferWriter.getBuilder().getLength())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can use File::writeAllText().

@@ -0,0 +1,122 @@
#ifndef SLANG_CORE_PRETTY_WRITER_H
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put this file under compiler-core instead of core?

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 6, 2024

For formatting, you can post a message in this thread with the content "/format" and you will get a pull request to your branch for the formatting changes.

writer << ",\n\"bindings\": [\n";
writer.indent();

auto parameterCount = programReflection->getParameterCount();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line has a warning that breaks the build on CI:

warning C4456: declaration of 'parameterCount' hides previous local declaration

@expipiplus1
Copy link
Collaborator

It's not necessary to do, but it would be nice if this handled passing - to output the json to stdout.

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 7, 2024

The code still needs to be formated.

@tareksander
Copy link
Contributor Author

/format

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 7, 2024

There are linking errors.

@expipiplus1
Copy link
Collaborator

/format

(@tareksander, I just fixed a bug where non-write users couldn't trigger the formatting bot, I've done it now but it should work for you in the future)

@slangbot
Copy link
Contributor

slangbot commented Nov 8, 2024

🌈 Formatted, please merge the changes from this PR

@tareksander
Copy link
Contributor Author

Would also be good if we can also get this added to our COM-based compilation API, in the form of IComponentType::getReflectionJson(IBlob** outBlob).

I don't have enough knowledge of that API to do that.

There are linking errors.

Weird. slang-reflection-test links with the slang library, but the linker can't find Slang::emitReflectionJSON.

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 8, 2024

This is because slang is compiled as an DLL, and functions in a dll is generally not visible to other projects, unless you explicitly export it. You need to expose it in slang-deprecated.h similar to other reflection functions.

// declare in slang-deprecated.h and implement as follows:

SLANG_API SlangResult spReflection_ToJson(SlangReflection* reflection, SlangCompileRequest* request, IBlob** outBlob)
{
       PrettyWriter writer;
       emitReflectionJSON(request, reflection, writer);
       *outBlob = StringBlob::moveCreate(writer.getBuilder());
       return SLANG_OK;
}

Then you will be able to call this from reflection-test.

@tareksander
Copy link
Contributor Author

This is because slang is compiled as an DLL, and functions in a dll is generally not visible to other projects, unless you explicitly export it. You need to expose it in slang-deprecated.h similar to other reflection functions.

Weird, since I'm on Linux, and symbols should be visible by default. That fixed it anyways, thanks.

Now I get a segfault in spReflection_GetTypeParameterCount during the test, no idea why.

@tareksander tareksander force-pushed the reflection-compiler-option branch from 97181f6 to 5bb5b80 Compare November 9, 2024 11:17
include/slang.h Outdated
@@ -884,6 +884,7 @@ typedef uint32_t SlangSizeT;
DumpWarningDiagnostics,
InputFilesRemain,
EmitIr, // bool
EmitReflectionJSON, // bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break binary compatibility. New enum values should be added at the end.

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 11, 2024

The failures are due to C++ object lifetime issues.

You need to do:

*outObject = StringBlob::moveCreate(...).detach();

Otherwise the object will be deleted upon return.

Also changed the callsite to use

ComPtr<ISlangBlob> b;
spReflection_ToJson(..., b.writeRef());

instead of using raw ISlangBlob* pointer for b to ensure consistent life-time management.

I pushed the changes to your branch.

@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Nov 11, 2024
@csyonghe
Copy link
Collaborator

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@csyonghe csyonghe merged commit 7e51180 into shader-slang:master Nov 11, 2024
14 checks passed
@csyonghe
Copy link
Collaborator

Everything looks good after fixing the memory management issue.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to generate binding and layout information for slangc
4 participants