Merge branch 'DRTVWR-589-nested-table-bug' into DRTVWR-589

3 files changed, 465 insertions, 8 deletions

diff --git a/indra/llcommon/hexdump.h b/indra/llcommon/hexdump.h
new file mode 100755
index 0000000000..234168cd61
--- /dev/null
+++ b/indra/llcommon/hexdump.h
@@ -0,0 +1,106 @@
+/**
+ * @file   hexdump.h
+ * @author Nat Goodspeed
+ * @date   2023-10-03
+ * @brief  iostream manipulators to stream hex, or string with nonprinting chars
+ * 
+ * $LicenseInfo:firstyear=2023&license=viewerlgpl$
+ * Copyright (c) 2023, Linden Research, Inc.
+ * $/LicenseInfo$
+ */
+
+#if ! defined(LL_HEXDUMP_H)
+#define LL_HEXDUMP_H
+
+#include <cctype>
+#include <iomanip>
+#include <iostream>
+#include <string_view>
+
+namespace LL
+{
+
+// Format a given byte string as 2-digit hex values, no separators
+// Usage: std::cout << hexdump(somestring) << ...
+class hexdump
+{
+public:
+    hexdump(const std::string_view& data):
+        hexdump(data.data(), data.length())
+    {}
+
+    hexdump(const char* data, size_t len):
+        hexdump(reinterpret_cast<const unsigned char*>(data), len)
+    {}
+
+    hexdump(const std::vector<unsigned char>& data):
+        hexdump(data.data(), data.size())
+    {}
+
+    hexdump(const unsigned char* data, size_t len):
+        mData(data, data + len)
+    {}
+
+    friend std::ostream& operator<<(std::ostream& out, const hexdump& self)
+    {
+        auto oldfmt{ out.flags() };
+        auto oldfill{ out.fill() };
+        out.setf(std::ios_base::hex, std::ios_base::basefield);
+        out.fill('0');
+        for (auto c : self.mData)
+        {
+            out << std::setw(2) << unsigned(c);
+        }
+        out.setf(oldfmt, std::ios_base::basefield);
+        out.fill(oldfill);
+        return out;
+    }
+
+private:
+    std::vector<unsigned char> mData;
+};
+
+// Format a given byte string as a mix of printable characters and, for each
+// non-printable character, "\xnn"
+// Usage: std::cout << hexmix(somestring) << ...
+class hexmix
+{
+public:
+    hexmix(const std::string_view& data):
+        mData(data)
+    {}
+
+    hexmix(const char* data, size_t len):
+        mData(data, len)
+    {}
+
+    friend std::ostream& operator<<(std::ostream& out, const hexmix& self)
+    {
+        auto oldfmt{ out.flags() };
+        auto oldfill{ out.fill() };
+        out.setf(std::ios_base::hex, std::ios_base::basefield);
+        out.fill('0');
+        for (auto c : self.mData)
+        {
+            // std::isprint() must be passed an unsigned char!
+            if (std::isprint(static_cast<unsigned char>(c)))
+            {
+                out << c;
+            }
+            else
+            {
+                out << "\\x" << std::setw(2) << unsigned(c);
+            }
+        }
+        out.setf(oldfmt, std::ios_base::basefield);
+        out.fill(oldfill);
+        return out;
+    }
+
+private:
+    std::string mData;
+};
+
+} // namespace LL
+
+#endif /* ! defined(LL_HEXDUMP_H) */
diff --git a/indra/newview/llluamanager.cpp b/indra/newview/llluamanager.cpp
index 61c0ecf1ed..de02fe4646 100644
--- a/indra/newview/llluamanager.cpp
+++ b/indra/newview/llluamanager.cpp
@@ -28,6 +28,7 @@
 #include "llviewerprecompiledheaders.h"
 #include "llluamanager.h"
 
+#include "hexdump.h"
 #include "llerror.h"
 #include "lleventcoro.h"
 #include "lleventfilter.h"
@@ -1070,6 +1071,122 @@ void lua_pushstdstring(lua_State* L, const std::string& str)
     lua_pushlstring(L, str.c_str(), str.length());
 }
 
+// Usage:  std::cout << lua_what(L, stackindex) << ...;
+// Reports on the Lua value found at the passed stackindex.
+// If cast to std::string, returns the corresponding string value.
+class lua_what
+{
+public:
+    lua_what(lua_State* state, int idx):
+        L(state),
+        index(idx)
+    {}
+
+    friend std::ostream& operator<<(std::ostream& out, const lua_what& self)
+    {
+        switch (lua_type(self.L, self.index))
+        {
+        case LUA_TNONE:
+            // distinguish acceptable but non-valid index
+            out << "none";
+            break;
+
+        case LUA_TNIL:
+            out << "nil";
+            break;
+
+        case LUA_TBOOLEAN:
+        {
+            auto oldflags { out.flags() };
+            out << std::boolalpha << lua_toboolean(self.L, self.index);
+            out.flags(oldflags);
+            break;
+        }
+
+        case LUA_TNUMBER:
+            out << lua_tonumber(self.L, self.index);
+            break;
+
+        case LUA_TSTRING:
+            out << std::quoted(lua_tostdstring(self.L, self.index));
+            break;
+
+        case LUA_TUSERDATA:
+        {
+            const size_t maxlen = 20;
+            auto binlen{ lua_rawlen(self.L, self.index) };
+            LLSD::Binary binary(std::min(maxlen, binlen));
+            std::memcpy(binary.data(), lua_touserdata(self.L, self.index), binary.size());
+            out << LL::hexdump(binary);
+            if (binlen > maxlen)
+            {
+                out << "...(" << (binlen - maxlen) << " more)";
+            }
+            break;
+        }
+
+        case LUA_TLIGHTUSERDATA:
+            out << lua_touserdata(self.L, self.index);
+            break;
+
+        default:
+            // anything else, don't bother trying to report value, just type
+            out << lua_typename(self.L, lua_type(self.L, self.index));
+            break;
+        }
+        return out;
+    }
+
+    operator std::string() const { return stringize(*this); }
+
+private:
+    lua_State* L;
+    int index;
+};
+
+// Usage:  std::cout << lua_stack(L) << ...;
+// Reports on the contents of the Lua stack.
+// If cast to std::string, returns the corresponding string value.
+class lua_stack
+{
+public:
+    lua_stack(lua_State* state):
+        L(state)
+    {}
+
+    friend std::ostream& operator<<(std::ostream& out, const lua_stack& self)
+    {
+        const char* sep = "stack: [";
+        for (int index = 1; index <= lua_gettop(self.L); ++index)
+        {
+            out << sep << lua_what(self.L, index);
+            sep = ", ";
+        }
+        out << ']';
+        return out;
+    }
+
+    operator std::string() const { return stringize(*this); }
+
+private:
+    lua_State* L;
+};
+
+// log exit from any block declaring an instance of DebugExit, regardless of
+// how control leaves that block
+struct DebugExit
+{
+    DebugExit(const std::string& name): mName(name) {}
+    DebugExit(const DebugExit&) = delete;
+    DebugExit& operator=(const DebugExit&) = delete;
+    ~DebugExit()
+    {
+        LL_DEBUGS("Lua") << "exit " << mName << LL_ENDL;
+    }
+
+    std::string mName;
+};
+
 // By analogy with existing lua_tomumble() functions, return an LLSD object
 // corresponding to the Lua object at stack index 'index' in state L.
 // This function assumes that a Lua caller is fully aware that they're trying
@@ -1087,6 +1204,9 @@ void lua_pushstdstring(lua_State* L, const std::string& str)
 // reached by that block raises a Lua error.
 LLSD lua_tollsd(lua_State* L, int index)
 {
+    LL_DEBUGS("Lua") << "lua_tollsd(" << index << ") of " << lua_gettop(L) << " stack entries: "
+                     << lua_what(L, index) << LL_ENDL;
+    DebugExit log_exit("lua_tollsd()");
     switch (lua_type(L, index))
     {
     case LUA_TNONE:
@@ -1158,17 +1278,41 @@ LLSD lua_tollsd(lua_State* L, int index)
         // - LLSD::Real with integer value returns as LLSD::Integer.
         // - LLSD::UUID, LLSD::Date and LLSD::URI all convert to Lua string,
         //   and so return as LLSD::String.
+        // - Lua does not store any table key whose value is nil. An LLSD
+        //   array with isUndefined() entries produces a Lua table with
+        //   "holes" in the int key sequence; this converts back to an LLSD
+        //   array containing corresponding isUndefined() entries -- except
+        //   when one or more of the final entries isUndefined(). These are
+        //   simply dropped, producing a shorter LLSD array than the original.
+        // - For the same reason, any keys in an LLSD map whose value
+        //   isUndefined() are simply discarded in the converted Lua table.
+        //   This converts back to an LLSD map lacking those keys.
+        // - If it's important to preserve the original length of an LLSD
+        //   array whose final entries are undefined, or the full set of keys
+        //   for an LLSD map some of whose values are undefined, store an
+        //   LLSD::emptyArray() or emptyMap() instead. These will be
+        //   represented in Lua as empty table, which should convert back to
+        //   undefined LLSD. Naturally, though, those won't survive a second
+        //   round trip.
 
         // This is the most important of the luaL_checkstack() calls because a
         // deeply nested Lua structure will enter this case at each level, and
         // we'll need another 2 stack slots to traverse each nested table.
         luaL_checkstack(L, 2, nullptr);
+        // BEFORE we push nil to initialize the lua_next() traversal, convert
+        // 'index' to absolute! Our caller might have passed a relative index;
+        // we do, below: lua_tollsd(L, -1). If 'index' is -1, then when we
+        // push nil, what we find at index -1 is nil, not the table!
+        index = lua_absindex(L, index);
+        LL_DEBUGS("Lua") << "checking for empty table" << LL_ENDL;
         lua_pushnil(L);             // first key
+        LL_DEBUGS("Lua") << lua_stack(L) << LL_ENDL;
         if (! lua_next(L, index))
         {
             // it's a table, but the table is empty -- no idea if it should be
             // modeled as empty array or empty map -- return isUndefined(),
             // which can be consumed as either
+            LL_DEBUGS("Lua") << "empty table" << LL_ENDL;
             return {};
         }
         // key is at stack index -2, value at index -1
@@ -1177,6 +1321,8 @@ LLSD lua_tollsd(lua_State* L, int index)
         LuaPopper popper(L, 2);
         // Remember the type of the first key
         auto firstkeytype{ lua_type(L, -2) };
+        LL_DEBUGS("Lua") << "table not empty, first key type " << lua_typename(L, firstkeytype)
+                         << LL_ENDL;
         switch (firstkeytype)
         {
         case LUA_TNUMBER:
@@ -1254,6 +1400,7 @@ LLSD lua_tollsd(lua_State* L, int index)
                 // crazy key territory.
                 return luaL_error(L, "Gaps in Lua table too large for conversion to LLSD array");
             }
+            LL_DEBUGS("Lua") << "collected " << keys.size() << " keys, max " << highkey << LL_ENDL;
             // right away expand the result array to the size we'll need
             LLSD result{ LLSD::emptyArray() };
             result[highkey - 1] = LLSD();
@@ -1263,8 +1410,10 @@ LLSD lua_tollsd(lua_State* L, int index)
             {
                 // key at stack index -2, value at index -1
                 // We've already validated lua_tointegerx() for each key.
+                auto key{ lua_tointeger(L, -2) };
+                LL_DEBUGS("Lua") << "key " << key << ':' << LL_ENDL;
                 // Don't forget to subtract 1 from Lua key for LLSD subscript!
-                result[LLSD::Integer(lua_tointeger(L, -2)) - 1] = lua_tollsd(L, -1);
+                result[LLSD::Integer(key) - 1] = lua_tollsd(L, -1);
                 // remove value, keep key for next iteration
                 lua_pop(L, 1);
             }
@@ -1283,8 +1432,10 @@ LLSD lua_tollsd(lua_State* L, int index)
                     return luaL_error(L, "Cannot convert %s map key to LLSD",
                                       lua_typename(L, mapkeytype));
                 }
-                
-                result[lua_tostdstring(L, -2)] = lua_tollsd(L, -1);
+
+                auto key{ lua_tostdstring(L, -2) };
+                LL_DEBUGS("Lua") << "map key " << std::quoted(key) << ':' << LL_ENDL;
+                result[key] = lua_tollsd(L, -1);
                 // remove value, keep key for next iteration
                 lua_pop(L, 1);
             } while (lua_next(L, index) != 0);
diff --git a/indra/newview/tests/llluamanager_test.cpp b/indra/newview/tests/llluamanager_test.cpp
index e5e06b095c..6433ff1118 100644
--- a/indra/newview/tests/llluamanager_test.cpp
+++ b/indra/newview/tests/llluamanager_test.cpp
@@ -15,12 +15,17 @@
 #include "../newview/llluamanager.h"
 // STL headers
 // std headers
+#include <vector>
 // external library headers
 // other Linden headers
 #include "../test/lltut.h"
 #include "llapp.h"
+#include "lldate.h"
 #include "llevents.h"
 #include "lleventcoro.h"
+#include "llsdutil.h"
+#include "lluri.h"
+#include "lluuid.h"
 #include "stringize.h"
 #include "../llcommon/tests/StringVec.h"
 
@@ -32,6 +37,17 @@ public:
     bool frame()   override { return true; }
 };
 
+template <typename CALLABLE>
+auto listener(CALLABLE&& callable)
+{
+    return [callable=std::forward<CALLABLE>(callable)]
+    (const LLSD& data)
+    {
+        callable(data);
+        return false;
+    };
+}
+
 /*****************************************************************************
 *   TUT
 *****************************************************************************/
@@ -56,11 +72,8 @@ namespace tut
         LLEventStream replypump("testpump");
         LLTempBoundListener conn(
             replypump.listen("test<1>",
-                             [&posts](const LLSD& post)
-                             {
-                                 posts.push_back(post.asString());
-                                 return false;
-                             }));
+                             listener([&posts](const LLSD& data)
+                             { posts.push_back(data.asString()); })));
         const std::string lua(
             "-- test post_on,listen_events,await_event\n"
             "post_on('testpump', 'entry')\n"
@@ -98,4 +111,191 @@ namespace tut
         llcoro::suspend();
         ensure_equals("post_on() sequence", posts, expected);
     }
+
+    void from_lua(const std::string& desc, const std::string_view& construct, const LLSD& expect)
+    {
+        LLSD fromlua;
+        LLEventStream replypump("testpump");
+        LLTempBoundListener conn(
+            replypump.listen("llluamanager_test",
+                             listener([&fromlua](const LLSD& data){ fromlua = data; })));
+        const std::string lua(stringize(
+            "-- test LLSD synthesized by Lua\n",
+            // we expect the caller's Lua snippet to construct a Lua object
+            // called 'data'
+            construct, "\n"
+            "post_on('testpump', data)\n"
+        ));
+        LLLUAmanager::runScriptLine(lua);
+        // At this point LLLUAmanager::runScriptLine() has launched a new C++
+        // coroutine to run the passed Lua snippet, but that coroutine hasn't
+        // yet had a chance to run. Poke the coroutine scheduler until the Lua
+        // script has sent its data.
+        for (int i = 0; i < 10 && fromlua.isUndefined(); ++i)
+        {
+            llcoro::suspend();
+        }
+        // We woke up again ourselves because the coroutine running Lua has
+        // finished.
+        ensure_equals(desc, fromlua, expect);
+    }
+
+    template<> template<>
+    void object::test<2>()
+    {
+        set_test_name("LLSD from Lua");
+        from_lua("nil", "data = nil", LLSD());
+        from_lua("true", "data = true", true);
+        from_lua("false", "data = false", false);
+        from_lua("int", "data = 17", 17);
+        from_lua("real", "data = 3.14", 3.14);
+        from_lua("string", "data = 'string'", "string");
+        // can't synthesize Lua userdata in Lua code: that can only be
+        // constructed by a C function
+        from_lua("empty table", "data = {}", LLSD());
+        from_lua("nested empty table", "data = { 1, 2, 3, {}, 5 }",
+                 llsd::array(1, 2, 3, LLSD(), 5));
+        from_lua("nested non-empty table", "data = { 1, 2, 3, {a=0, b=1}, 5 }",
+                 llsd::array(1, 2, 3, llsd::map("a", 0, "b", 1), 5));
+    }
+
+    void round_trip(const std::string& desc, const LLSD& send, const LLSD& expect)
+    {
+        LLSD reply;
+        LLEventStream replypump("testpump");
+        LLTempBoundListener conn(
+            replypump.listen("llluamanager_test",
+                             listener([&reply](const LLSD& post){ reply = post; })));
+        const std::string lua(
+            "-- test LLSD round trip\n"
+            "callback = function(pump, data)\n"
+            "    -- just echo the data we received\n"
+            "    post_on('testpump', data)\n"
+            "end\n"
+            "replypump, cmdpump = listen_events(callback)\n"
+            "post_on('testpump', replypump)\n"
+            "await_event(replypump)\n"
+        );
+        LLLUAmanager::runScriptLine(lua);
+        // At this point LLLUAmanager::runScriptLine() has launched a new C++
+        // coroutine to run the passed Lua snippet, but that coroutine hasn't
+        // yet had a chance to run. Poke the coroutine scheduler until the Lua
+        // script has sent its reply pump name.
+        for (int i = 0; i < 10 && reply.isUndefined(); ++i)
+        {
+            llcoro::suspend();
+        }
+        // We woke up again ourselves because the coroutine running Lua has
+        // reached the await_event() call, which suspends the calling C++
+        // coroutine (including the Lua code running on it) until we post
+        // something to that reply pump.
+        auto luapump{ reply.asString() };
+        reply.clear();
+        LLEventPumps::instance().post(luapump, send);
+        // The C++ coroutine running the Lua script is now ready to run. Run
+        // it so it will echo the LLSD back to us.
+        llcoro::suspend();
+        ensure_equals(desc, reply, expect);
+    }
+
+    // Define an RTItem to be used for round-trip LLSD testing: what it is,
+    // what we send to Lua, what we expect to get back. They could be the
+    // same.
+    struct RTItem
+    {
+        RTItem(const std::string& name, const LLSD& send, const LLSD& expect):
+            mName(name),
+            mSend(send),
+            mExpect(expect)
+        {}
+        RTItem(const std::string& name, const LLSD& both):
+            mName(name),
+            mSend(both),
+            mExpect(both)
+        {}
+
+        std::string mName;
+        LLSD mSend, mExpect;
+    };
+
+    template<> template<>
+    void object::test<3>()
+    {
+        set_test_name("LLSD round trip");
+        LLSD::Binary binary{ 3, 1, 4, 1, 5, 9, 2, 6, 5 };
+        const char* uuid{ "01234567-abcd-0123-4567-0123456789ab" };
+        const char* date{ "2023-10-04T21:06:00Z" };
+        const char* uri{ "https://secondlife.com/index.html" };
+        std::vector<RTItem> items{
+            RTItem("undefined", LLSD()),
+            RTItem("true", true),
+            RTItem("false", false),
+            RTItem("int", 17),
+            RTItem("real", 3.14),
+            RTItem("int real", 27.0, 27),
+            RTItem("string", "string"),
+            RTItem("binary", binary),
+            RTItem("empty array", LLSD::emptyArray(), LLSD()),
+            RTItem("empty map", LLSD::emptyMap(), LLSD()),
+            RTItem("UUID", LLUUID(uuid), uuid),
+            RTItem("date", LLDate(date), date),
+            RTItem("uri", LLURI(uri), uri)
+            };
+        // scalars
+        for (const auto& item: items)
+        {
+            round_trip(item.mName, item.mSend, item.mExpect);
+        }
+
+        // array
+        LLSD send_array{ LLSD::emptyArray() }, expect_array{ LLSD::emptyArray() };
+        for (const auto& item: items)
+        {
+            send_array.append(item.mSend);
+            expect_array.append(item.mExpect);
+        }
+        // exercise the array tail trimming below
+        send_array.append(items[0].mSend);
+        expect_array.append(items[0].mExpect);
+        // Lua takes a table value of nil to mean: don't store this key. An
+        // LLSD array containing undefined entries (converted to nil) leaves
+        // "holes" in the Lua table. These will be converted back to undefined
+        // LLSD entries -- except at the end. Trailing undefined entries are
+        // simply omitted from the table -- so the table converts back to a
+        // shorter LLSD array. We've constructed send_array and expect_array
+        // according to 'items' above -- but truncate from expect_array any
+        // trailing entries whose mSend will map to Lua nil.
+        while (expect_array.size() > 0 &&
+               send_array[expect_array.size() - 1].isUndefined())
+        {
+            expect_array.erase(expect_array.size() - 1);
+        }
+        round_trip("array", send_array, expect_array);
+
+        // map
+        LLSD send_map{ LLSD::emptyMap() }, expect_map{ LLSD::emptyMap() };
+        for (const auto& item: items)
+        {
+            send_map[item.mName] = item.mSend;
+            // see comment in the expect_array truncation loop above --
+            // Lua never stores table entries with nil values
+            if (item.mSend.isDefined())
+            {
+                expect_map[item.mName] = item.mExpect;
+            }
+        }
+        round_trip("map", send_map, expect_map);
+
+        // deeply nested map: exceed Lua's default stack space (20),
+        // i.e. verify that we have the right checkstack() calls
+        for (int i = 0; i < 20; ++i)
+        {
+            LLSD new_send_map{ send_map }, new_expect_map{ expect_map };
+            new_send_map["nested map"] = send_map;
+            new_expect_map["nested map"] = expect_map;
+            send_map = new_send_map;
+            expect_map = new_expect_map;
+        }
+        round_trip("nested map", send_map, expect_map);
+    }
 } // namespace tut