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: add KEEP_FOR_RE() macro to SKSE/Impl/PCH.h #21

Closed
wants to merge 1 commit into from

Conversation

ThirdEyeSqueegee
Copy link
Member

Problem:

  • Non-concrete classes are optimized out of the pdb when building the library

Proposed solution:

  • Add the provided KEEP_FOR_RE() macro to each reversed class (possibly via a GitHub action) to concretize it and prevent it from being optimized out during debug builds

@ThirdEyeSqueegee ThirdEyeSqueegee added the enhancement New feature or request label Sep 8, 2023
@shadeMe
Copy link
Contributor

shadeMe commented Sep 8, 2023

Just use the OPT:NOREF linker flag instead? This looks very hacky and intrusive.

@alandtse
Copy link
Contributor

alandtse commented Sep 8, 2023

I believe the issue is that compiler flags wouldn't solve the problem because there's no guarantee that classes with 0 references will be included even with the flags. Happy to be wrong.

@gottyduke
Copy link
Contributor

I don't think PR has any meaning at this moment, it should be more of an issue to discuss.
The macro will likely intefere with vtable entries for derived class when it's enabled.
Besides that, you are adding another .gitignore file in the sub directory, which is identical to the root .gitignore.
Closing this until there're actual RE structures to test with and a more viable solution is agreed upon.

@gottyduke gottyduke closed this Sep 9, 2023
@ThirdEyeSqueegee
Copy link
Member Author

ThirdEyeSqueegee commented Sep 9, 2023

This PR was created specifically to allow discussing the problem--please refrain from flippantly closing PRs in the future. Alan and Nightfallstorm spent a significant amount of time dealing with this for CommonLibSSE, this PR was made so the given implementation and alternatives could be discussed for CommonLibSF.

@gottyduke
Copy link
Contributor

Which is why I suggested to open an issue for discussion, the PR hanging here without anything to test is impractical, the macro snippet could be posted within a code block instead of forcing to update a PCH file.
If you are taking this as a serious concern, then it's even more vital to discuss thoroughly and actually test the code locally before trying to merge into the repo? Is this PR tested with any given RE class for pdb generation? Does this solution intefere with vtable entry for derived classes? That's what we should discuss before submitting code, no?

But if you are very certain about this, please re-open the PR instead of an issue, and I presume you'd also handle the auto CI action as well.

@ThirdEyeSqueegee
Copy link
Member Author

You're playing semantics here, it doesn't matter if it's raised as a PR or an issue. The point was to discuss the problem. As far as I know, the given solution does work and was tested by Alan and Nightfall.

@ThirdEyeSqueegee ThirdEyeSqueegee deleted the feature/concretization branch September 9, 2023 15:39
@alandtse
Copy link
Contributor

alandtse commented Sep 9, 2023

Nightfall tested in SSE. The reason a PR is better is someone can check it out or cherry pick it to test it. You can't do that with issues.

@gottyduke
Copy link
Contributor

You're playing semantics here,

Please do not say that, I'm contributing here with good intentions only, and that accusation is not okay. I'll reiterate my statement, if you are very certain about this, please re-open the PR instead of an issue, and I presume you'd also handle the auto CI action as well.

@ThirdEyeSqueegee
Copy link
Member Author

Lol no one is questioning your intentions, I'm just gonna make a new PR for this later bc this thread is a bit of a dumpster fire

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants