anderslanglands / alShaders2

BSD 3-Clause "New" or "Revised" License
75 stars 38 forks source link

Minimum gcc version? #4

Closed noizfactory closed 7 years ago

noizfactory commented 7 years ago

Hi Jonah,

I'm compiling this for a lower gcc version (4.6) but get some warnings during the compile. Although the shader compiles, I wanted to confirm if I can ignore these warnings:

warning: cannot pass objects of non-POD type ‘class AtString’ through ‘...’; call will abort at runtime

Thanks, Sachin

anderslanglands commented 7 years ago

Hi Sachin, I've built cryptomatte using gcc 4.8.5. Do you have a line number where that warning came from? On Fri, 14 Jul 2017 at 18:23, Sachin Shrestha notifications@github.com wrote:

Hi Jonah,

I'm compiling this for a lower gcc version (4.6) but get some warnings during the compile. Although the shader compiles, I wanted to confirm if I can ignore these warnings:

warning: cannot pass objects of non-POD type ‘class AtString’ through ‘...’; call will abort at runtime

Thanks, Sachin

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/anderslanglands/alShaders2/issues/4, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2IXS6xuuxwTh6rWiEvnAbu7bJY6CkYks5sNwlGgaJpZM4OX39T .

noizfactory commented 7 years ago

Hi Anders,

Sorry, I culled the line numbers by mistake. These are the line numbers:

cryptomatte_shader.cpp:69: warning: cannot pass objects of non-POD type ‘class AtString’ through ‘...’; call will abort at runtime

cryptomatte_shader.cpp:72: warning: cannot pass objects of non-POD type ‘class AtString’ through ‘...’; call will abort at runtime

The one I compiled with 4.6 is able to load in maya and kick seems to prints its attributes as well but I was just wondering if these warnings will cause issues later.

Thanks, Sachin

anderslanglands commented 7 years ago

Yeah looks spurious to me. I wouldn't worry about it but if it does randomly abort, let us know.... On Fri, 14 Jul 2017 at 18:49, Sachin Shrestha notifications@github.com wrote:

Hi Anders,

Sorry, I culled the line numbers by mistake. These are the line numbers:

cryptomatte_shader.cpp:69: warning: cannot pass objects of non-POD type ‘class AtString’ through ‘...’; call will abort at runtime

cryptomatte_shader.cpp:72: warning: cannot pass objects of non-POD type ‘class AtString’ through ‘...’; call will abort at runtime

The one I compiled with 4.6 is able to load in maya and kick seems to prints its attributes as well but I was just wondering if these warnings will cause issues later.

Thanks, Sachin

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/anderslanglands/alShaders2/issues/4#issuecomment-315284863, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2IXTF1Jkrq1CgF5MqXxTV-5Feg3MC7ks5sNw9fgaJpZM4OX39T .

noizfactory commented 7 years ago

Sure, Anders.

noizfactory commented 7 years ago

Hi Anders,

While kick is able to load the cryptomatte shader node and show me its nodes if I pass the -nodes flag, if I try to use this within Maya then it crashes the Maya session. This is the crash-log I get:

****
* Arnold 5.0.2.0.wip [0cdaba84] linux clang-4.0.0 oiio-1.7.15 osl-1.9.0 vdb-4.0.0 clm-1.0.3.513 rlm-12.2.2 2017/07/11 10:44:04
* CRASHED in Update at 00:00:00
* signal caught: SIGILL -- Illegal instruction (illegal operand)
*
* backtrace:
*  0 0x00002aaae483e137 [libai.so       ]
*  1 0x00002b8f2891e4ff [libpthread.so.0]
>> 2 0x00002aac1fe31d22 [cryptomatte.so ] Update(AtNode*)                                [cryptomatte_shader.cpp:0]
*  3 0x00002aac1fe31d22 [cryptomatte.so ]                                                [cryptomatte_shader.cpp:0]
*  4 0x00002aaae47d1d28 [libai.so       ]
*  5 0x00002aaaee608586 [libmtoa_api.so ] CRenderSession::InteractiveRenderThread(void*)
*  6 0x00002b8f28916850 [libpthread.so.0]
*  7 0x0000003548ce890c [libc.so.6      ] clone
*
* loaded modules:
*    0x00002aaae4000000  libai.so
*    0x00002b8f2890f000  libpthread.so.0
*    0x00002aac1fde4000  cryptomatte.so
*    0x00002aaaee572000  libmtoa_api.so
*    0x0000003548c00000  libc.so.6
*
* memory: VM 77676 MB, RSS 980 MB, 1 page faults
****
Stack trace:
  ../overrides/arnold/5.0.0.0/thirdparty/alShaders-linux-2.0.0-ai5.0.1.0/bin/cryptomatte.so(+0x4dd23) [0x2aac1fe31d23]
  ../bin/libai.so(+0x7e1a1e) [0x2aaae47e1a1e]
  ../bin/libai.so(+0x7d1d29) [0x2aaae47d1d29]
  CRenderSession::InteractiveRenderThread(void*)
  /lib64/libpthread.so.0(+0x3549407851) [0x2b8f28916851]
  clone

The MtoA version is 5.0.2.0 from the daily builds so not sure if that has something to do with it. I'm trying it with an official release build right now to check if that crashes as well.

noizfactory commented 7 years ago

Ok, it crashes with the previous official MtoA 2.0.1.0 as well. The crashlog is the same. From a workflow perspective, these are the steps I'm following in Maya:

I hit render and it crashes Maya. I tried only a scene ass export and that goes through but if I kick that ass it crashes again with the same errors.

noizfactory commented 7 years ago

Hi Anders,

I compiled the shader on a CentOS 7 machine (with gcc 4.8.5) and that works fine without any crashes. So it seems like those lines that Arnold compiler complained about are probably causing the problem. If I explicitly convert the string returns from AiNodeGetStr() and recompile with 4.6 then I don't get the compilation warnings and the kick does not crash when rendering with cryptomatte either. Maybe its an issue with the AtArray constructor? Here's the patch for your reference:

diff --git a/cryptomatte/cryptomatte_shader.cpp b/cryptomatte/cryptomatte_shader.cpp
index 5a8df23..28efdef 100644
--- a/cryptomatte/cryptomatte_shader.cpp
+++ b/cryptomatte/cryptomatte_shader.cpp
@@ -65,11 +65,11 @@ node_update
                                                   AiNodeGetBool(node, "strip_mat_namespaces"));

    AtArray* uc_aov_array = AiArray(4, 1, AI_TYPE_STRING, 
-      AiNodeGetStr(node, "user_crypto_aov_0"), AiNodeGetStr(node, "user_crypto_aov_1"), 
-      AiNodeGetStr(node, "user_crypto_aov_2"), AiNodeGetStr(node, "user_crypto_aov_3"));
+      AiNodeGetStr(node, "user_crypto_aov_0").c_str(), AiNodeGetStr(node, "user_crypto_aov_1").c_str(), 
+      AiNodeGetStr(node, "user_crypto_aov_2").c_str(), AiNodeGetStr(node, "user_crypto_aov_3").c_str());
    AtArray* uc_src_array = AiArray(4, 1, AI_TYPE_STRING, 
-      AiNodeGetStr(node, "user_crypto_src_0"), AiNodeGetStr(node, "user_crypto_src_1"), 
-      AiNodeGetStr(node, "user_crypto_src_2"), AiNodeGetStr(node, "user_crypto_src_3"));
+      AiNodeGetStr(node, "user_crypto_src_0").c_str(), AiNodeGetStr(node, "user_crypto_src_1").c_str(), 
+      AiNodeGetStr(node, "user_crypto_src_2").c_str(), AiNodeGetStr(node, "user_crypto_src_3").c_str());

    CryptomatteData_setup_all(data, AiNodeGetStr(node, "aov_crypto_asset"),
                           AiNodeGetStr(node, "aov_crypto_object"),
jonahfriedman commented 7 years ago

Thanks for the patch. I think this is something to raise with Solid Angle.

ThiagoIze commented 7 years ago

The fix Sachin proposed is the correct way to code this up. This is mentioned in the AtString documentation as well:

* AtString will automatically call c_str() in most situations and so can be
* automatically used in places that expect a \c char*.  However, functions
* with a variable number of arguments (printf, AiMsg, ...) will require
* manually converting to char* with the c_str() member function.
anderslanglands commented 7 years ago

Thanks Thiago. I'm curious why gcc 4.8 works fine then?

Sachin, is there a reason you're stuck on 4.6? 4.8 will be the minimum I think I can guarantee it will work on as although our use of C++11 features is small at the moment it's likely to grow and while the list for 4.6 is reasonably complete there may still be dragons lurking. On Wed, 19 Jul 2017 at 05:04, ThiagoIze notifications@github.com wrote:

The fix Sachin proposed is the correct way to code this up. This is mentioned in the AtString documentation as well:

  • AtString will automatically call c_str() in most situations and so can be
  • automatically used in places that expect a \c char*. However, functions
  • with a variable number of arguments (printf, AiMsg, ...) will require
  • manually converting to char* with the c_str() member function.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/anderslanglands/alShaders2/issues/4#issuecomment-316130292, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2IXdv8DYgg7QNw1JALv7Hn8ql8Nlvtks5sPOWjgaJpZM4OX39T .

ThiagoIze commented 7 years ago

Passing a non-POD type as a variadic arg is undefined behavior in C++03, so we can't really complain if it doesn't work. In fact, it's possible an even newer version of gcc could stop working, so I would recommend sticking to using c_str() just to be safe.

Now, in the C++11 case I think this might be valid, though it does mention it's "conditionally-supported". From the spec ( C++11 5.2.2/7):

Passing a potentially-evaluated argument of class type (Clause 9) having a non-trivial copy constructor, a non-trivial move constructor, or a non-trivial destructor, with no corresponding parameter, is conditionally-supported with implementation-defined semantics.

AtString looks to have trivial versions of all these functions (right?) so I suppose it should be ok. Perhaps gcc 4.8 made sure this would work with C++11 and figured they might as well keep things simple and make the C++03 code path match?

In this particular case, if the compiler pretended it was a POD type it should actually work fine even with gcc 4.6, but I suspect what is happening is that the 4.6 compiler is doing some sort of optimization for non-POD, which it's allowed to do, which causes this crash.

noizfactory commented 7 years ago

Hi Anders,

We are on lower gcc due to legacy support issues. However, since my last post, I have compiled with gcc 4.8.2 using the factory code without the patch and it compiles fine and renders fine as well. However, based on Thiago's recommendation I'm still going to use the patched version internally here to avoid any potential issues later.

Thanks for looking into this guys!

Cheers, Sachin

anderslanglands commented 7 years ago

Could you submit a PR with your patch please? Would be good to include this just in case. On Wed, 19 Jul 2017 at 21:58, Sachin Shrestha notifications@github.com wrote:

Hi Anders,

We are on lower gcc due to legacy support issues. However, since my last post, I have compiled with gcc 4.8.2 using the factory code without the patch and it compiles fine and renders fine as well. However, based on Thiago's recommendation I'm still going to use the patched version internally here to avoid any potential issues later.

Thanks for looking into this guys!

Cheers, Sachin

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/anderslanglands/alShaders2/issues/4#issuecomment-316335199, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2IXTqKPk4JduGuI43bNVrR5U1K9--zks5sPdMygaJpZM4OX39T .

jonahfriedman commented 7 years ago

This will be in the next release.

noizfactory commented 6 years ago

Adding a build for gcc 4.6.1 for some users on the arnold mailing list.

alShaders-linux-2.0.0-ai5.0.1.0_gcc_4.6.1.zip