Fix illegal function pointer conversions

[bcoconni]

In preparation to the resolution of the undefined behavior reported in #834 (comment), this PR removes most of the pointer conversions that are currently done to tie properties.

• In most cases, the compiler is able to automatically determine the function pointer types and the conversions have simply been removed.
• In cases where a nullptr is given for the getters, the type names in the template need to be explicitly specified and this removes the need for explicit conversions.
• In the remaining cases, this PR replaces the C cast by C++'s reinterpret_cast<>. These casts are precisely where the problem reported in issue JSBSim for SPEC CPUv8 #834 happens. A further PR will be needed to remove them. In the meantime they can easily be spotted in the source code by doing a simple search of the reinterpret_cast.

[Copilot]

Pull Request Overview

This PR removes most C-style function pointer conversions in favor of letting the compiler automatically determine function pointer types or using C++ reinterpret_cast<> for necessary conversions. The changes prepare the codebase for addressing undefined behavior related to function pointer type conversions (issue #834).

• Eliminates most C-style casts for function pointers by removing unnecessary explicit conversions
• Adds template type parameters to PropertyManager->Tie() calls where nullptr getters are used
• Replaces remaining C casts with explicit reinterpret_cast<> to identify problematic conversions

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/models/flight_control/FGPID.cpp	Replaced typedef and C-cast with template parameters for PropertyManager::Tie
src/models/atmosphere/FGWinds.cpp	Removed C-style casts and added template parameters, used reinterpret_cast for remaining conversions
src/models/atmosphere/FGStandardAtmosphere.cpp	Changed typedef to using declaration and replaced C-casts with reinterpret_cast
src/models/FGPropulsion.cpp	Removed typedefs and added template parameters for nullptr getter cases
src/models/FGPropagate.cpp	Removed typedefs and C-casts, added template parameters and reinterpret_cast for enum conversions
src/models/FGOutput.cpp	Replaced typedef and C-cast with template parameters
src/models/FGMassBalance.cpp	Removed typedefs and C-casts, replaced numeric indices with enum constants
src/models/FGExternalReactions.cpp	Removed typedef and C-casts for function pointers
src/models/FGAuxiliary.cpp	Removed typedefs and C-casts for function pointers
src/models/FGAircraft.cpp	Removed typedef and C-casts for function pointers
src/models/FGAerodynamics.cpp	Removed typedef and C-casts for function pointers
src/models/FGAccelerations.cpp	Removed using declaration and C-casts for function pointers
src/FGFDMExec.cpp	Removed typedef, added template parameters, and replaced C-casts with proper types

[Copilot]

[nitpick] The use of reinterpret_cast for enum-to-int conversion may indicate a design issue. Consider if the PropertyManager API could be enhanced to handle enum types directly to avoid the need for casting.

[codecov]

Codecov Report

❌ Patch coverage is 92.51337% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.76%. Comparing base (1f7bceb) to head (707005f).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/models/FGExternalReactions.cpp	0.00%	6 Missing ⚠️
src/models/FGPropulsion.cpp	0.00%	4 Missing ⚠️
src/input_output/FGPropertyManager.h	0.00%	2 Missing ⚠️
src/models/atmosphere/FGWinds.cpp	96.42%	1 Missing ⚠️
src/models/flight_control/FGPID.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1304      +/-   ##
==========================================
- Coverage   24.76%   24.76%   -0.01%     
==========================================
  Files         169      169              
  Lines       19605    19608       +3     
==========================================
  Hits         4856     4856              
- Misses      14749    14752       +3     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[seanmcleod70]

When I saw the undefined behavior reported in #834 (comment) last night I tried a quick test to see what would be involved in terms of removing the function pointer casts for the FGWinds turbulence type enum.

So changing:

PropertyManager->Tie("atmosphere/turb-type", this, (PMFt)&FGWinds::GetTurbType, (PMFi)&FGWinds::SetTurbType);

to:

PropertyManager->Tie("atmosphere/turb-type", this, &FGWinds::GetTurbType, &FGWinds::SetTurbType);

The first compiler issue was that there was no matching PropertyTraits for the turbulence type enum.

jsbsim/src/simgear/props/props.hxx

Lines 180 to 197 in c8af924

	template<typename T> struct PropertyTraits;
	#define DEFINTERNALPROP(TYPE, PROP) \
	template<> \
	struct PropertyTraits<TYPE> \
	{ \
	static const Type type_tag = PROP; \
	enum { Internal = 1 }; \
	}
	DEFINTERNALPROP(bool, BOOL);
	DEFINTERNALPROP(int, INT);
	DEFINTERNALPROP(long, LONG);
	DEFINTERNALPROP(float, FLOAT);
	DEFINTERNALPROP(double, DOUBLE);
	DEFINTERNALPROP(const char *, STRING);
	DEFINTERNALPROP(const char[], STRING);
	#undef DEFINTERNALPROP

So I added a PropertyTraits to FGWinds.cpp.

template<> struct simgear::props::PropertyTraits<JSBSim::FGWinds::tType> {
  static const simgear::props::Type type_tag = simgear::props::Type::INT; 
  enum { Internal = 1 };
};

Which resulted in a successful compile, but then a linker error since it couldn't find a matching getValue method, so I added the following to FGWinds.cpp to fix the linker issue.

template<>
inline JSBSim::FGWinds::tType getValue<JSBSim::FGWinds::tType>(const SGPropertyNode* node) 
{ return (JSBSim::FGWinds::tType)node->getIntValue(); }

Not sure how feasible it would be to have something like:

DEFINTERNALPROP(<any enum type>, INT);

[seanmcleod70]

Hmm, thought I could potentially use std::is_enum_v<T> to provide a generic getValue() function for all enum types.

template <typename T, typename = std::enable_if_t<std::is_enum_v<T>>>
inline T getValue<T>(const SGPropertyNode* node) {
  return reinterpret_cast<T>(node->getIntValue());
}

But hitting a compiler error.

1>FGWinds.cpp
1>C:\source\jsbsim\src\models\atmosphere\FGWinds.cpp(76,10): error C2768: 'getValue': illegal use of explicit template arguments

[bcoconni]

Hmm, thought I could potentially use std::is_enum_v<T> to provide a generic getValue() function for all enum types.

Yeah, I'm stumbling upon the same problem. However I've managed to shorten some of your code:

template<> struct simgear::props::PropertyTraits<JSBSim::FGWinds::tType> {
  static const simgear::props::Type type_tag = simgear::props::Type::INT; 
  enum { Internal = 1 };
};

can be replaced by

namespace simgear::props {
template<> struct PropertyTraits<JSBSim::FGWinds::tType> : public PropertyTraits<int> {};
};

This avoids knowing the internals of PropertyTraits (in case someone adds a new member in the official SimGear).

[bcoconni]

Some progress on my side. The following works for any enum:

template<typename T> T getValue(const SGPropertyNode* node)
{
  static_assert(std::is_enum_v<T>);
  return static_cast<T>(node->getIntValue());
}

As simple as that. This is working because props.hxx does not define the general case:

jsbsim/src/simgear/props/props.hxx

Lines 1837 to 1844 in c8af924

	// Convenience functions for use in templates
	template<typename T>
	#if PROPS_STANDALONE
	T
	#else
	typename boost::disable_if<boost::is_enum<T>, T>::type
	#endif
	getValue(const SGPropertyNode*);

Note that the static_assert serves no other purpose than making sure that this template is not instantiated for anything other than an enum.

[bcoconni]

@seanmcleod70 I've just pushed a new commit that implements your ideas about the specialization of PropertyTraits. It also includes some modifications from myself to shorten the code.

Let me know what you think about it.

[seanmcleod70]

can be replaced by

namespace simgear::props {
template<> struct PropertyTraits<JSBSim::FGWinds::tType> : public PropertyTraits<int> {};
};

Yep, my ultimate aim/hope was that there might be a way to turn that into something like this with some template magic.

namespace simgear::props {
template<> struct PropertyTraits<is_any_enum> : public PropertyTraits<int> {};
};

@bcoconni I pushed Update Comment just as your comment came in above, so hadn't taken a look at the commit at the time I sent my comment.

[seanmcleod70]

Just looking at your latest commit and I noticed the previous 2 commits are from June 8th. So you had been working on those before this SPEC specific issue came up? I did think it was a very quick response from you to the SPEC issue in terms of the amount of code you'd changed in such a short space of time given it was only reported around midnight our time last night 😉

[seanmcleod70]

Hmm, so I was looking at your getValue() implementation and your comment above:

This is working because props.hxx does not define the general case

template<typename T> T getValue(const SGPropertyNode* node)
{
  static_assert(is_enum_v<T>); // Guard against misusing template instantiation.
  return static_cast<T>(node->getIntValue());
}

In Visual Studio it looks like PROPS_STANDALONE is defined:

So we end up with a declaration of:

template<typename T> T getValue(const SGPropertyNode* node)

But no definition until your definition?

And it looks like in the case that PROPS_STANDALONE isn't defined then there is some boost specific checks to exclude enum types?

[bcoconni]

Just looking at your latest commit and I noticed the previous 2 commits are from June 8th.

Yeah. The bug that @heshpdx fixed in PR #1291 regarding the property accelerations/uidot-ft_sec2 and its siblings was very annoying. This should have been detected a long time ago by the compiler if we were not using these ugly casts in the calls to FGPropertyManager::Tie. This is the reason why I had initiated this change back in June.

Now this is the second time we are bitten by these conversions so it is some more incentive to remove the typedef's and casts.

[bcoconni]

In Visual Studio it looks like PROPS_STANDALONE is defined:

It is unconditionally defined for all platforms:

jsbsim/src/simgear/props/props.hxx

Lines 14 to 16 in c8af924

	#ifndef PROPS_STANDALONE
	#define PROPS_STANDALONE 1
	#endif

But no definition until your definition?

None that I could find.

And it looks like in the case that PROPS_STANDALONE isn't defined then there is some boost specific checks to exclude enum types?

Yep but I am not sure that it would be a problem since we are defining a more general case. But the compiler may complain that the 2 definitions overlap.

[seanmcleod70]

@bcoconni any reason why in the commit you added the getValue template function to FGWinds.cpp as opposed to adding it to props.hxx or some other header file (given FlightGear simgear integration)? I've tested moving it to props.hxx and it compiles and links fine.

I also tried out the following C++20 option, i.e. using requires as opposed to the static_assert. But I'm not sure when we'll be upgrading to C++20 😉

template<typename T> requires std::is_enum_v<T> T getValue(const SGPropertyNode* node)
{
    return static_cast<T>(node->getIntValue());
}

[seanmcleod70]

Talking of C++20 I just tried the following in FGWinds.cpp and it compiles and links.

namespace simgear::props {
  //template<> struct PropertyTraits<JSBSim::FGWinds::tType> : public PropertyTraits<int> {};

  template<typename T> requires std::is_enum_v<T> struct PropertyTraits<T> : public PropertyTraits<int> {};
};

[seanmcleod70]

Okay, back to C++17 and using your idea of using static_assert I've just tested the following which compiles and links.

namespace simgear::props {
  //template<> struct PropertyTraits<JSBSim::FGWinds::tType> : public PropertyTraits<int> {};

  //template<typename T> requires std::is_enum_v<T> struct PropertyTraits<T> : public PropertyTraits<int> {};

  template<typename T> struct PropertyTraits : public PropertyTraits<int> {
    static_assert(std::is_enum_v<T>, "PropertyTraits specialization for enum types only");
  };
};

To be clear, all my testing so far has simply been to confirm that the code compiles and links successfully.

[seanmcleod70]

I've put the following into FGPropertyManager.h, i.e. none of this code in FGWinds.cpp or props.hxx and it compiles and links.

namespace simgear::props {
  template<typename T> struct PropertyTraits : public PropertyTraits<int> {
    static_assert(std::is_enum_v<T>, "PropertyTraits specialization for enum types only");
  };
}

template<typename T> T getValue(const SGPropertyNode* node)
{
  static_assert(std::is_enum_v<T>, "getValue specialization for enum types only"); // Guard against misusing template instantiation.
  return static_cast<T>(node->getIntValue());
}

[bcoconni]

I've put the following into FGPropertyManager.h, i.e. none of this code in FGWinds.cpp or props.hxx and it compiles and links.

namespace simgear::props {
 template<typename T> struct PropertyTraits : public PropertyTraits<int> {
   static_assert(std::is_enum_v<T>, "PropertyTraits specialization for enum types only");
 };
}

Hmm, I am not too comfortable with the default implementation of a template that is inheriting from one of its own specialization. This is very close to a recursive definition and I am concerned that later in the future some pedantic compiler may reject that creative construction.

[seanmcleod70]

Could revert to my earlier use of mirroring the #define DEFINEINTERNALPROP implementation.

  template<typename T> struct PropertyTraits
  {
    static_assert(std::is_enum_v<T>, "PropertyTraits specialization for enum types only");

    static const simgear::props::Type type_tag = simgear::props::Type::INT;
    enum { Internal = 1 };
  };

It does as you mentioned at the time tie us into the internal implementation of PropertyTraits in that if/when we take an updated simgear drop if they change the internals of PropertyTraits then we may need to update this code to match the changes.

[seanmcleod70]

I did some very basic testing. Modified 737_cruise.xml to write to the wind turbulence enum during the trim event.

<set name="atmosphere/turb-type" value="2"/>

Took a look at the stack trace in the debugger to confirm it had the correct function type signature etc.

And also modified 737_cruise.xml to periodically report the wind turbulence enum.

<property>atmosphere/turb-type</property>

Ran both a debug and release build, although only MSVC.

[bcoconni]

any reason why in the commit you added the getValue template function to FGWinds.cpp as opposed to adding it to props.hxx or some other header file (given FlightGear simgear integration)? I've tested moving it to props.hxx and it compiles and links fine.

@seanmcleod70 We cannot add new content to props.hxx because, as I have explained in #834 (comment), JSBSim is using FlightGear/SimGear own versions of the files props.[ch]xx when embedded in FlightGear and it would not include our code for getValue<T>.

However I have pushed a new commit to this PR to move the template getValue<T> to FGPropertyManager.h as per your suggestion. This allows removing the reinterpret_cast<>'s from FGPropagate.cpp at the cost of a one-liner to specialize simgear::props::PropertyTraits<> for FGPropagate::eIntegrateType.

After this commit getValue<T> is available anywhere in JSBSim, provided FGPropertyManager.h is included. And PropertyTraits<> is specialized "locally" i.e. only in the source file where it is needed. Given that specializing PropertyTraits<> is just one line of code, this seems like a good compromise to me.

Let me know what you think.

[seanmcleod70]

@seanmcleod70 We cannot add new content to props.hxx because, as I have explained in #834 (comment), JSBSim is using FlightGear/SimGear own versions of the files

But I switched to putting the code into FGPropertyManager.h very soon afterwards I asked the original question.

I've put the following into FGPropertyManager.h, i.e. none of this code in FGWinds.cpp or props.hxx and it compiles and links.

Plus you even commented on my comment mentioning the move to FGPropertyManager.h a week or so ago 😉

I'll take a look at the latest commit.

[seanmcleod70]

Was a bit surprised that there were only 3 enum types across all the property bindings.

template<> struct PropertyTraits<JSBSim::FGPropagate::eIntegrateType> : public PropertyTraits<int> {};
template<> struct PropertyTraits<JSBSim::FGWinds::tType> : public PropertyTraits<int> {};
template<> struct PropertyTraits<JSBSim::FGWinds::eGustFrame> : public PropertyTraits<int> {};

And PropertyTraits<> is specialized "locally" i.e. only in the source file where it is needed. Given that specializing PropertyTraits<> is just one line of code, this seems like a good compromise to me.

Yep. I thought there were a whole host of enum types spread across all the different files with property bindings so I thought having a single version would help. Plus, no need to have another location with the internals of PropertyTraits specified with your approach.

So all good from my side in terms of your latest commits.

[bcoconni]

But I switched to putting the code into FGPropertyManager.h very soon afterwards I asked the original question.
[...]
Plus you even commented on my comment mentioning the move to FGPropertyManager.h a week or so ago 😉

Oops ! Sorry for the confusion.

[bcoconni]

So all good from my side in terms of your latest commits.

Great ! The PR is now merged.