Codecov Report

❌ Patch coverage is 70.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.87%. Comparing base (5200260) to head (bf00d70).
⚠️ Report is 3 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1312      +/-   ##
==========================================
+ Coverage   24.81%   24.87%   +0.05%     
==========================================
  Files         169      169              
  Lines       19631    19655      +24     
==========================================
+ Hits         4872     4889      +17     
- Misses      14759    14766       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Just had a quick initial look. So it looks like the issue here is we're dealing with methods that return an enum, whereas the last 2 PRs were dealing with methods that took an enum as an argument?

Actually not a member method returning an enum, rather it's a member field of type enum?

When it segfaults, is there any debug info mentioning exactly which missing virtual function it was trying to call?

The segfault comes during untie(), where the value gets resolved.

inline int
SGPropertyNode::get_int () const
{
  if (_tied)
      return (static_Cast<SGRawValue<int>*>(_value.val)->getValue();
  else
    return _local_val.int_val;
}

From what I can tell, LLVM is optimizing away the vtable from the actual SGRawValueMethods type. Casting to a SGRawValue<int> parent type is not the same, and expects a vtable. Making an SGRawValueMethodsEnum<> that inherits from SGRawValue<int> gets the compiler into the place that it understands that these types are connected... Deriving from SGRawValue<Enum> is not the same as SGRawValue<int>.

As for the other calls, I believe it is also coming back to the same root cause, but the cast of the pointer from Enum* to int* is sufficient to avoid the issue.

I have to say that I am not comfortable with this PR.

Unless proven mistaken, the code in SGPropertyNode::get_int is legal C++ code. So we are in a completely different ballpark than PRs #1304 and #1311. And I am struggling to see why we should change JSBSim code to accomodate extremely aggressive optimizations from LLVM which make legal C++ code fail ?

I mean before merging the PRs #1304 and #1311, we were using reinterpret_cast<> under the hood and this casting is notoriously known to generate undefined behavior, in particular preventing the compiler to check that the conversion is legal.

In SGPropertyNode::get_int, the code is using static_cast<> and compilers would complain if the conversion was illegal. So unless someone proves us that the code in SGPropertyNode::get_int is invalid or undefined behaviour, I am challenging the validity of the LLVM optimization.

Static casting of a parent pointer to a derived pointer is fine, as long as you are casting to a type that matches the underlying data. _value.val is a SGRaw*, so it is legal to cast to SGRawValue<int>*, as long as the pointer is pointing to an object of that type.

SGRawValue<Enum> is not the same type as SGRawValue<int>, and so this is not valid C++.

Consider the following:

struct Base {
    Base() = default;
    virtual ~Base() = default;
};

struct Derived1 : public Base  {
    virtual ~Derived1() = default;
};

struct Derived2 : public Base  {
    virtual ~Derived2() = default;
};


int main(void) {
    Base * v = new Derived1();
    Derived2 *v2 = static_cast<Derived2*>(v);

    Derived1 d1;
    Derived2 * pd1 = static_cast<Derived2*>(&d1);

    return 0;
}

$ g++ ./test.cc
./test.cc: In function ‘int main()’:
./test.cc:20:22: error: invalid ‘static_cast’ from type ‘Derived1*’ to type ‘Derived2*’
   20 |     Derived2 * pd1 = static_cast<Derived2*>(&d1);
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

The first cast is not shown as an error, because we have laundered the type through the base pointer. When directly casting, without laundering the pointer, the compiler knows that it is an invalid cast.

Thanks @bramoore. I stand corrected.

And I'm wondering if using dynamic_cast<> in SGPropertyNode::get_int and its sibling would not be some good practice.

   if (_tied) {
     auto raw_value = dynamic_cast<SGRawValue<int>*>(_value.val);
     assert(raw_value); // Intercepts errors when compiled with NDEBUG.
     return raw_value->getValue();
   }
   else 
     return _local_val.int_val; 

This change would be implemented on our copy of SimGear only but this would help making sure that future changes are not introducing similar issues.

Yep, just confirmed that dynamic_cast<SGRawValue<int>*>(_value.val) does return nullptr in this case.

Any reason not to also ask the maintainers of SimGear to switch to using dynamic_cast for these cases?

Yep, just confirmed that dynamic_cast<SGRawValue<int>*>(_value.val) does return nullptr in this case.

Nice ! At least we now have a way to intercept these errors without recurring to some specific optimization from one compiler. However as far as I am aware dynamic_cast<> is using more CPU cycles than static_cast<> and I have some vague recollection that JSBSim is already spending much time in the SGPropertyNode code.

Any reason not to also ask the maintainers of SimGear to switch to using dynamic_cast for these cases?

Why not ? However I am not sure they have creative usages of SGRawValue<T> just as we have.

Yep, static_cast<> is literally 3 mov operations:

       auto raw_value = static_cast<SGRawValue<int>*>(_value.val);
00000001402AB0CA  mov         rax,qword ptr [this]  
00000001402AB0CF  mov         rax,qword ptr [rax+98h]  
00000001402AB0D6  mov         qword ptr [rsp+20h],rax 
        return raw_value->getValue();

Whereas dynamic_cast calls into the C++ runtime's dynamic cast function.

        auto raw_value = dynamic_cast<SGRawValue<int>*>(_value.val);
00000001402AB0CA  mov         dword ptr [rsp+20h],0  
00000001402AB0D2  lea         r9,[SGRawValue<int> `RTTI Type Descriptor' (01404AE258h)]  
00000001402AB0D9  lea         r8,[SGRaw `RTTI Type Descriptor' (01404AD8F8h)]  
00000001402AB0E0  xor         edx,edx  
00000001402AB0E2  mov         rax,qword ptr [this]  
00000001402AB0E7  mov         rcx,qword ptr [rax+98h]  
00000001402AB0EE  call        __RTDynamicCast (0140365CB2h)  
00000001402AB0F3  mov         qword ptr [rsp+30h],rax  
        return raw_value->getValue();

Which is at least another 2 function calls deep.

So maybe we should only ifdef the dynamic_cast for debug builds, and make sure we include some debug builds that run the existing test cases etc.

Checking in to see what the next steps are. Is there anything more you need from us?

I think we should go-ahead with this PR, but leave the dynamic_cast suggestion until later, to decide whether to use it at all, or only in debug builds, do some performance testing on it etc. @bcoconni your thoughts?

I think we should go-ahead with this PR, but leave the dynamic_cast suggestion until later, to decide whether to use it at all, or only in debug builds, do some performance testing on it etc.

Yep ! I was about to say the same thing. So I 've pushed the "merge" button.