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

Explore alternative force-export method for inline methods using forward declarations #171

Open
PathogenDavid opened this issue Feb 27, 2021 · 4 comments
Labels
Area-Transformation Issues concerning Biohazrd transformations Concept-InlineExpectation Issues concerning problems around C++'s expectation for something to be inlined. Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output

Comments

@PathogenDavid
Copy link
Member

While adding a workaround for ocornut/imgui#3850 I stumbled upon an alternative solution for (potentially?) forcing inline methods to export. Turns out you can add __declspec(dllexport) to a forward declaration and the members will be exported. (See here)

However, I did not test to see if this worked for inline methods.

I seem to recall trying this with inline functions and it didn't work, but this could potentially cut down on the noise in certain libraries.

@PathogenDavid PathogenDavid added Area-Transformation Issues concerning Biohazrd transformations Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output Concept-InlineExpectation Issues concerning problems around C++'s expectation for something to be inlined. labels Feb 27, 2021
@PathogenDavid
Copy link
Member Author

PathogenDavid commented Feb 27, 2021

Fixing this sort-of requires fixing #172

Edit: Actually, maybe not since the inline reference file is typically not part of the Biohazrd build. Either way we could special-case things just for this issue if it came to it. Fixing that issue is actually a bit of a pain to do without increasing the complexity of translation unit parsing (which is already pretty complicated.)

@PathogenDavid
Copy link
Member Author

PathogenDavid commented Feb 27, 2021

I seem to recall trying this with inline functions and it didn't work

I messed around a little on Godbolt and I think the issue was I tried putting the inline forward-declaration after the function definition. Either that or I tried it to export a single inline method out of a class after it was defined...

Edit: Although in the case of functions this is easier said than done since you need access to all of the parameter types which quickly becomes a pain.

@PathogenDavid
Copy link
Member Author

Womp-womp. Specifying dllexport on both the struct and a method within it is illegal for whatever reason:

struct __declspec(dllexport) Hello;

struct Hello
{
    __declspec(dllexport) void Test(); // error C2487: 'Test': member of dll interface class may not be declared with dll interfac
};

@PathogenDavid
Copy link
Member Author

PathogenDavid commented Feb 27, 2021

The good news is it did work besides that, and it exported the constructor/destructor too. Unfortunately I don't think this approach is likely to be realistic except for libraries which aren't even annotated with __declspec(dllexport) unless we can also disable all dllexport annotations and add all the nonsense required to also forward declare functions. (In my experience, libraries tend to be all-or-nothing with their exports and use a single macro for everything.)

This might be viable as an alternate mode or as a mode that is opportunistically used when none of the individual methods are marked as dllexport.

However, since this method is problematic and the current solution works pretty well as it is, I don't think I can consider this a high priority. In the case of ImGui specifically, I'm actually more inclined to offer a PR to normalize how IMGUI_API is applied to structs.

As visible in the diff below, this also requires providing a mechanism for writing to the area before includes in CppCodeWriter since the forward-declarations have to come before any definitions. Although I think in a real implementation I'd emit a separate header file that gets included from the export helper with custom sorting making it the firstest.

Also note that the below implementation is hard coded to only apply to ImDrawCmd after I discovered the double-export issue above.

diff --git a/Biohazrd.Utilities/InlineExportHelper.cs b/Biohazrd.Utilities/InlineExportHelper.cs
index df60031..231e044 100644
--- a/Biohazrd.Utilities/InlineExportHelper.cs
+++ b/Biohazrd.Utilities/InlineExportHelper.cs
@@ -8,6 +8,7 @@ using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
 using System.Runtime.InteropServices;
+using System.Text;
 using System.Threading;
 
 namespace Biohazrd.Utilities
@@ -18,11 +19,31 @@ namespace Biohazrd.Utilities
         private bool Used = false;
         private readonly List<TranslatedFunction> FunctionsExportedViaFunctionPointer = new();
         private readonly List<TranslatedFunction> FunctionsExportedViaTrampoline = new();
+        private readonly List<TranslatedRecord> RecordsExportedViaForwardDeclaration = new();
         private volatile int NextTrampolineId = 0;
-        private readonly CppCodeWriter Writer;
+        private readonly CppCodeWriterWithHax Writer;
+
+        [ProvidesOutputSessionFactory]
+        private class CppCodeWriterWithHax : CppCodeWriter
+        {
+            public StringBuilder BeforeIncludes = new();
+
+            protected CppCodeWriterWithHax(OutputSession session, string filePath)
+                : base(session, filePath)
+            { }
+
+            private static OutputSession.WriterFactory<CppCodeWriterWithHax> FactoryMethod => (session, filePath) => new CppCodeWriterWithHax(session, filePath);
+
+            protected override void WriteBetweenHeaderAndCode(StreamWriter writer)
+            {
+                writer.WriteLine(BeforeIncludes.ToString());
+
+                base.WriteBetweenHeaderAndCode(writer);
+            }
+        }
 
         public InlineExportHelper(OutputSession session, string filePath)
-            => Writer = session.Open<CppCodeWriter>(filePath);
+            => Writer = session.Open<CppCodeWriterWithHax>(filePath);
 
         public void ForceInclude(string filePath, bool systemInclude = false)
         {
@@ -85,6 +106,15 @@ namespace Biohazrd.Utilities
             return declaration;
         }
 
+        protected override TransformationResult TransformRecord(TransformationContext context, TranslatedRecord declaration)
+        {
+            if (declaration.Name == "ImDrawCmd")
+            {
+                RecordsExportedViaForwardDeclaration.Add(declaration);
+            }
+            return declaration;
+        }
+
         protected override TransformationResult TransformFunction(TransformationContext context, TranslatedFunction declaration)
         {
             // Edge case: All static non-method functions need to be exported by a trampoline (even the non-inline ones.)
@@ -106,6 +136,9 @@ namespace Biohazrd.Utilities
             if (declaration.Declaration is not FunctionDecl functionDecl)
             { return declaration; }
 
+            if (declaration.Declaration is CXXMethodDecl && context.ParentDeclaration?.Name == "ImDrawCmd")
+            { return declaration; }
+
             // Private/protected methods can't be exported using the techniques we use today
             // https://github.com/InfectedLibraries/Biohazrd/issues/162
             if (functionDecl is CXXMethodDecl methodDecl && methodDecl.Access != CX_CXXAccessSpecifier.CX_CXXPublic)
@@ -147,6 +180,11 @@ namespace Biohazrd.Utilities
         {
             const string dummyNamespaceName = "____BiohazrdInlineExportHelpers";
 
+            foreach (TranslatedRecord record in RecordsExportedViaForwardDeclaration)
+            {
+                Writer.BeforeIncludes.AppendLine($"{record.Kind.ToString().ToLowerInvariant()} __declspec(dllexport) {record.Original.Name};");
+            }
+
             //=====================================================================================
             // Handle function pointer exports
             //=====================================================================================

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Transformation Issues concerning Biohazrd transformations Concept-InlineExpectation Issues concerning problems around C++'s expectation for something to be inlined. Concept-OutputFriendliness Issues concerning the friendliness of using the Biohazrd output
Projects
None yet
Development

No branches or pull requests

1 participant