Obfuscation naming issue fix (#1676)

* #include an absolute path didn't work - because paths were taken to always be relative.

* Work around for issue with obfuscation (and lack of name hints) leading to names in output not being correctly uniquified.

* Improve appendChar
Remove unrequired memory juggling to scrub names.

* Remove test code.

* Small fixes in comments and method called.

* Remove linkage decoration on functions that are specialized.

* Obfuscation naming with specialization test.

* Fix instruction deletion.

Co-authored-by: Tim Foley <tfoleyNV@users.noreply.github.com>

7 files changed, 141 insertions, 72 deletions

diff --git a/source/core/slang-string.cpp b/source/core/slang-string.cpp
index 6d06eb1c6..4b1ec4c84 100644
--- a/source/core/slang-string.cpp
+++ b/source/core/slang-string.cpp
@@ -362,15 +362,25 @@ namespace Slang
         }
     }
 
-    void String::append(char chr)
+    void String::appendChar(char c)
     {
-        append(&chr, &chr + 1);
-    }
+        const auto oldLength = getLength();
+        const auto newLength = oldLength + 1;
+
+        ensureUniqueStorageWithCapacity(newLength);
 
+        // Since there must be space for at least one character, m_buffer cannot be nullptr
+        SLANG_ASSERT(m_buffer);
+        char* data = m_buffer->getData();
+        data[oldLength] = c;
+        data[newLength] = 0;
 
-    void String::appendChar(char chr)
+        m_buffer->length = newLength;
+    }
+
+    void String::append(char chr)
     {
-        append(&chr, &chr + 1);
+        appendChar(chr);
     }
 
     void String::append(String const& str)
diff --git a/source/slang/slang-emit-c-like.cpp b/source/slang/slang-emit-c-like.cpp
index 44564e575..009ca17f9 100644
--- a/source/slang/slang-emit-c-like.cpp
+++ b/source/slang/slang-emit-c-like.cpp
@@ -502,22 +502,24 @@ UInt CLikeSourceEmitter::getID(IRInst* value)
     return id;
 }
 
-String CLikeSourceEmitter::scrubName(const String& name)
+void CLikeSourceEmitter::appendScrubbedName(const UnownedStringSlice& name, StringBuilder& out)
 {
     // We will use a plain `U` as a dummy character to insert
     // whenever we need to insert things to make a string into
     // valid name.
     //
-    char const* dummyChar = "U";
+    const char dummyChar = 'U';
 
     // Special case a name that is the empty string, just in case.
     if(name.getLength() == 0)
-        return dummyChar;
-
+    {
+        out.appendChar(dummyChar);
+        return;
+    }
+     
     // Otherwise, we are going to walk over the name byte by byte
     // and write some legal characters to the output as we go.
-    StringBuilder sb;
-
+    
     if(getSourceLanguage() == SourceLanguage::GLSL)
     {
         // GLSL reserves all names that start with `gl_`,
@@ -525,7 +527,7 @@ String CLikeSourceEmitter::scrubName(const String& name)
         // our name start with a dummy character instead.
         if(name.startsWith("gl_"))
         {
-            sb.append(dummyChar);
+            out.appendChar(dummyChar);
         }
     }
 
@@ -534,7 +536,7 @@ String CLikeSourceEmitter::scrubName(const String& name)
     // to avoid an possible collision.
     if(name.startsWith("_S"))
     {
-        sb.Append(dummyChar);
+        out.appendChar(dummyChar);
     }
 
     // TODO: This is where we might want to consult
@@ -577,7 +579,7 @@ String CLikeSourceEmitter::scrubName(const String& name)
             // be a valid identifier in many target languages.
             if(prevChar == -1)
             {
-                sb.append(dummyChar);
+                out.appendChar(dummyChar);
             }
         }
         else if(c == '_')
@@ -604,8 +606,8 @@ String CLikeSourceEmitter::scrubName(const String& name)
             // Our solution for now will be very clumsy: we will
             // emit `x` and then the hexadecimal version of
             // the byte we were given.
-            sb.append("x");
-            sb.append(uint32_t((unsigned char) c), 16);
+            out.appendChar('x');
+            out.append(uint32_t((unsigned char) c), 16);
 
             // We don't want to apply the default handling below,
             // so skip to the top of the loop now.
@@ -613,11 +615,9 @@ String CLikeSourceEmitter::scrubName(const String& name)
             continue;
         }
 
-        sb.append(c);
+        out.appendChar(c);
         prevChar = c;
     }
-
-    return sb.ProduceString();
 }
 
 String CLikeSourceEmitter::generateEntryPointNameImpl(IREntryPointDecoration* entryPointDecor)
@@ -625,6 +625,50 @@ String CLikeSourceEmitter::generateEntryPointNameImpl(IREntryPointDecoration* en
     return entryPointDecor->getName()->getStringSlice();
 }
 
+String CLikeSourceEmitter::_generateUniqueName(const UnownedStringSlice& name)
+{
+    //
+    // We need to be careful that the name follows the rules of the target language,
+    // so there is a "scrubbing" step that needs to be applied here.
+    //
+    // We also need to make sure that the name won't collide with other declarations
+    // that might have the same name hint applied, so we will still unique
+    // them by appending the numeric ID of the instruction.
+    //
+    // TODO: Find cases where we can drop the suffix safely.
+    //
+    // TODO: When we start having to handle symbols with external linkage for
+    // things like DXIL libraries, we will need to *not* use the friendly
+    // names for stuff that should be link-able.
+    //
+    // The name we output will basically be:
+    //
+    //      <name>_<uniqueID>
+    //
+    // Except that we will "scrub" the name first,
+    // and we will omit the underscore if the (scrubbed)
+    // name hint already ends with one.
+    
+    StringBuilder sb;
+
+    appendScrubbedName(name, sb);
+
+    // Avoid introducing a double underscore
+    if (!sb.endsWith("_"))
+    {
+        sb.append("_");
+    }
+
+    String key = sb.ProduceString();
+    
+    UInt& countRef = m_uniqueNameCounters.GetOrAddValue(key, 0);
+    const UInt count = countRef;
+    countRef = count + 1;
+
+    sb.append(Int32(count));
+    return sb.ProduceString();
+}
+
 String CLikeSourceEmitter::generateName(IRInst* inst)
 {
     // If the instruction names something
@@ -667,62 +711,16 @@ String CLikeSourceEmitter::generateName(IRInst* inst)
     }
 
     // If we have a name hint on the instruction, then we will try to use that
-    // to provide the actual name in the output code.
-    //
-    // We need to be careful that the name follows the rules of the target language,
-    // so there is a "scrubbing" step that needs to be applied here.
-    //
-    // We also need to make sure that the name won't collide with other declarations
-    // that might have the same name hint applied, so we will still unique
-    // them by appending the numeric ID of the instruction.
-    //
-    // TODO: Find cases where we can drop the suffix safely.
-    //
-    // TODO: When we start having to handle symbols with external linkage for
-    // things like DXIL libraries, we will need to *not* use the friendly
-    // names for stuff that should be link-able.
-    //
+    // to provide the basis for the actual name in the output code.
     if(auto nameHintDecoration = inst->findDecoration<IRNameHintDecoration>())
     {
-        // The (non-obfuscated) name we output will basically be:
-        //
-        //      <nameHint>_<uniqueID>
-        //
-        // Except that we will "scrub" the name hint first,
-        // and we will omit the underscore if the (scrubbed)
-        // name hint already ends with one.
-        //
-        // The obfuscated name we output will simply be:
-        //
-        //      _<uniqueID>
-        //
-
-        StringBuilder sb;
-
-        String nameHint = nameHintDecoration->getName();
-        nameHint = scrubName(nameHint);
-
-        sb.append(nameHint);
-
-        // Avoid introducing a double underscore
-        if (!nameHint.endsWith("_"))
-        {
-            sb.append("_");
-        }
-        
-        String key = sb.ProduceString();
-        UInt count = 0;
-        m_uniqueNameCounters.TryGetValue(key, count);
-
-        m_uniqueNameCounters[key] = count+1;
-
-        sb.append(Int32(count));
-        return sb.ProduceString();
+        return _generateUniqueName(nameHintDecoration->getName());
     }
 
-    // If the instruction has a mangled name, then emit using that.
+    // If the instruction has a linkage decoration, just use that. 
     if(auto linkageDecoration = inst->findDecoration<IRLinkageDecoration>())
     {
+        // Just use the linkages mangled name directly.
         return linkageDecoration->getMangledName();
     }
 
diff --git a/source/slang/slang-emit-c-like.h b/source/slang/slang-emit-c-like.h
index 0544668cc..b9bddca69 100644
--- a/source/slang/slang-emit-c-like.h
+++ b/source/slang/slang-emit-c-like.h
@@ -154,7 +154,7 @@ public:
     UInt getID(IRInst* value);
 
         /// "Scrub" a name so that it complies with restrictions of the target language.
-    String scrubName(const String& name);
+    void appendScrubbedName(const UnownedStringSlice& name, StringBuilder& out);
 
     String generateName(IRInst* inst);
     virtual String generateEntryPointNameImpl(IREntryPointDecoration* entryPointDecor);
@@ -369,6 +369,9 @@ public:
         // Emit the argument list (including paranthesis) in a `CallInst`
     void _emitCallArgList(IRCall* call);
 
+
+    String _generateUniqueName(const UnownedStringSlice& slice);
+
         // Sort witnessTable entries according to the order defined in the witnessed interface type.
     List<IRWitnessTableEntry*> getSortedWitnessTableEntries(IRWitnessTable* witnessTable);
     
diff --git a/source/slang/slang-ir-inst-defs.h b/source/slang/slang-ir-inst-defs.h
index 7b26ad9cb..2cab493bd 100644
--- a/source/slang/slang-ir-inst-defs.h
+++ b/source/slang/slang-ir-inst-defs.h
@@ -578,15 +578,16 @@ INST(HighLevelDeclDecoration,               highLevelDecl,          1, 0)
     /* LinkageDecoration */
         INST(ImportDecoration, import, 1, 0)
         INST(ExportDecoration, export, 1, 0)
+    INST_RANGE(LinkageDecoration, ImportDecoration, ExportDecoration)
 
     /* Decorations for RTTI objects */
-        INST(RTTITypeSizeDecoration, RTTI_typeSize, 1, 0)
+    INST(RTTITypeSizeDecoration, RTTI_typeSize, 1, 0)
     INST(AnyValueSizeDecoration, AnyValueSize, 1, 0)
     INST(SequentialIDDecoration, SequentialIDDecoration, 1, 0)
 
     INST(TypeConstraintDecoration, TypeConstraintDecoration, 1, 0)
-    INST_RANGE(LinkageDecoration, ImportDecoration, ExportDecoration)
 
+    
     INST(BuiltinDecoration, BuiltinDecoration, 0, 0)
 
         /// The decorated instruction requires NVAPI to be included via prelude when compiling for D3D.
diff --git a/source/slang/slang-ir-specialize-function-call.cpp b/source/slang/slang-ir-specialize-function-call.cpp
index 7e6c819d3..531078fc7 100644
--- a/source/slang/slang-ir-specialize-function-call.cpp
+++ b/source/slang/slang-ir-specialize-function-call.cpp
@@ -762,6 +762,36 @@ struct FunctionParameterSpecializationContext
             oldFunc,
             newFunc);
 
+        // If we have added an Linkage decoration, we want to remove and destroy it,
+        // because the linkage should only be on the original function and
+        // not on the "torn off" copies made in this function.
+        //
+        // It *could* be argued that we don't want to duplicate the decoration instructions
+        // to begin with, just to throw them away. That may be true, but it's simpler to just remove
+        // them than filter out in cloning.
+
+        {
+            auto decorationList = newFunc->getDecorations();
+
+            const auto end = decorationList.end();
+            auto cur = decorationList.begin();
+
+            while(cur != end)
+            {
+                IRDecoration* decoration = *cur;
+
+                // We step before before the test/destroying to ensure cur is not pointing
+                // to a potentially destroyed instruction
+                ++cur;
+
+                if (as<IRLinkageDecoration>(decoration))
+                {
+                    decoration->removeAndDeallocate();
+                }
+            }
+        }
+
+
         // We are almost done at this point, except that `newFunc`
         // is lacking its parameters, as well as any of the body
         // instructions that we decided were needed during
diff --git a/tests/bugs/obfuscate-specialization-naming.slang b/tests/bugs/obfuscate-specialization-naming.slang
new file mode 100644
index 000000000..2511adbea
--- /dev/null
+++ b/tests/bugs/obfuscate-specialization-naming.slang
@@ -0,0 +1,23 @@
+//TEST(compute):COMPARE_COMPUTE_EX:-slang -compute -compile-arg -obfuscate -shaderobj 
+//TEST(compute, vulkan):COMPARE_COMPUTE_EX:-vk -compute -compile-arg -obfuscate -shaderobj
+
+//TEST_INPUT:ubuffer(data=[0 1 2 3], stride=4):name a
+RWStructuredBuffer<int> a;
+//TEST_INPUT:ubuffer(data=[0 2 4 6], stride=4):name b
+RWStructuredBuffer<int> b;
+
+//TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out,name output
+RWStructuredBuffer<int> output;
+
+int doThing(RWStructuredBuffer<int> buf, int index)
+{
+    return buf[index];
+}
+
+[numthreads(16, 1, 1)]
+void computeMain(uint3 dispatchThreadID : SV_DispatchThreadID)
+{
+    int tid = int(dispatchThreadID.x); 
+
+    output[tid] = doThing(a, tid) | doThing(b, tid);
+}
\ No newline at end of file
diff --git a/tests/bugs/obfuscate-specialization-naming.slang.expected.txt b/tests/bugs/obfuscate-specialization-naming.slang.expected.txt
new file mode 100644
index 000000000..4a384c4be
--- /dev/null
+++ b/tests/bugs/obfuscate-specialization-naming.slang.expected.txt
@@ -0,0 +1,4 @@
+0
+3
+6
+7