rjhogan / Adept-2

Combined array and automatic differentiation library in C++
http://www.met.reading.ac.uk/clouds/adept/
Apache License 2.0
163 stars 29 forks source link

static assert in SpecialMatrix::value_with_len_() #34

Closed Servomex-ivisser closed 3 months ago

Servomex-ivisser commented 3 months ago

I feel like I walked into one of your deliberate traps here.

I hit this error in building Adept-2 using an IAR toolchain / cmake, in method SpecialMatrix::value_with_len_()

Type value_with_len_(const Index& j, const Index& len) const {
  ADEPT_STATIC_ASSERT(false, CANNOT_USE_VALUE_WITH_LEN_ON_ARRAY_OF_RANK_OTHER_THAN_1);
  return 0;
}

File SpecialMatrix.h, line 1548 (version 2.1.1 release / tar)

This assertion is obviously unconditionally false. So not surprisingly, I get it during build time. This code is not seemingly conditionally compiled either, although I will admit I didn't track every #ifdef in the file.

Totally removing the method fixes this build issue, so I guess this method is a base class refinement? Changing the condition to true obviously also fixes the build issue.

So how should I be dodging this trap without making changes to the code?

Servomex-ivisser commented 3 months ago

I get this error while trying to compile file minimize_levenberg_marquardt.cpp

FAILED: third_party/adept-2.1.1/CMakeFiles/adept.dir/adept/minimize_levenberg_marquardt.o
C:\PROGRA~1\IARSYS~1\EMBEDD~1.2\arm\bin\iccarm.exe --c++ --silent E:\Projects\xxx\third_party\adept-2.1.1\adept\minimize_levenberg_marquardt.cpp -DARM_MATH_CM4 -DSTM32F415xx -DSTM32_THREAD_SAFE_STRATEGY=2 -DTX_INCLUDE_USER_DEFINE_FILE -DUSE_FULL_LL_DRIVER -DUSE_HAL_DRIVER -IE:\Projects\xxx\third_party\adept-2.1.1\include -r --cpu=Cortex-M4 --fpu=VFPv4_sp -e --endian=little --guard_calls --no_size_constraints -Ohs --debug --libc++ --no_rtti --dependencies=ns third_party\adept-2.1.1\CMakeFiles\adept.dir\adept\minimize_levenberg_marquardt.o.d -o third_party\adept-2.1.1\CMakeFiles\adept.dir\adept\minimize_levenberg_marquardt.o

        ADEPT_STATIC_ASSERT(false, CANNOT_USE_VALUE_WITH_LEN_ON_ARRAY_OF_RANK_OTHER_THAN_1);
        ^
"E:\Projects\xxx\third_party\adept-2.1.1\include\adept\SpecialMatrix.h",1548  Error[Pe135]:
          class "adept::SpecialMatrix<Type, Engine,
          IsActive>::value_with_len_(int const &, int const &)
          const::ERROR_CANNOT_USE_VALUE_WITH_LEN_ON_ARRAY_OF_RANK_OTHER_THAN_1"
          (declared at line 1548) has no member "STATIC_ASSERTION_HAS_FAILED"
ninja: build stopped: subcommand failed.

But I get the same if I temporarily knock this file out of the build process, so I think I can safely discount the theory that I am compiling files that shouldn't be.

rjhogan commented 3 months ago

Could you please tell me your platform and compiler? Is the problem when you just compile the adept code, or when you compile it into other software? This line has been here forever so it's surprising you get this problem if you are just compiling the adept package.

The idea of this function is to enable arrays to be indexed with other arrays or with the Matlab-like adept::end object, for example A(end) or A(range(3,end-2)). In this case, the "end" object does not yet know the size of the array that it is indexing - deeper within the implementation, this is worked out, and passed to indexing objects as the second argument of a value_withlen call. Now the point is that you can only index a dimension of an array with either a scalar or a one-dimensional array. Since a SpecialMatrix is two dimensions, you can only have called value_withlen if you used a SpecialMatrix to index an array. This would be an error, which is why it should always fail to compile. Are you able to provide me the line of the user-level code that induces this compile-time failure?

Servomex-ivisser commented 3 months ago

The target platform is an STM32 ARM Cortex-M4 processor. But the host platform on which we build the ELF file is Windows 10. The x-compilation process is facilitated by the IAR toolchain.

We're only just at the evaluating stage of the Adept-2 library. To that end, all I am building so far is the adept-2.1.1/test/test_misc.cpp file from your repository.

rjhogan commented 3 months ago

I've not used the IAR toolchain. What happens if you compile Adept simply by using ./configure then make on the target platform? Your failure message implies it failed in compiling minimize_levenberg_marquardt.cpp, which is part of the Adept library - it presumably didn't get on to test_misc.cpp if it couldn't compile the library first. Would be good to know the line of minimize_levenberg_marquardt.cpp where the problematic code occurs.

Servomex-ivisser commented 3 months ago

I've not used the IAR toolchain. What happens if you compile Adept simply by using ./configure then make on the target platform? Your failure message implies it failed in compiling minimize_levenberg_marquardt.cpp, which is part of the Adept library - it presumably didn't get on to test_misc.cpp if it couldn't compile the library first. Would be good to know the line of minimize_levenberg_marquardt.cpp where the problematic code occurs.

The target platform is an embedded ARM chip. It has no OS until we "burn" that into the chip (FTR: the OS is ThreadX). We can only build on the host platform. Running ./configure in Windows 10 is therefore pointless (plus: unsupported).

I can try building the library / tutorials on a Linux VM in a while. But I can only build for the host platform then. We have no license to use the IAR toolchain from a Linux host. Still, there may be some value in trying.

Servomex-ivisser commented 3 months ago

Well, you're right about it building fine on Linux. With your instructions and with my cmake set-up. But it shouldn't (surely?). There is some smoke and mirrors going on here.

If I change the method to:

Type value_with_len_(const Index& j, const Index& len) const {
  static_assert(false);
  return 0;
}

Then the build fails. So why does it not fail with your static assertion macro? After all, it passes false unconditionally. So the struct ERROR_CANNOT_USE_VALUE_WITH_LEN_ON_ARRAY_OF_RANK_OTHER_THAN_1 does not make the type STATIC_ASSERTION_HAS_FAILED.

rjhogan commented 3 months ago

I think the issue is with compiler behaviour. What we want is a compile-time error to be thrown if this function is called. The tricky thing is that a compile-time error is thrown if the function is compiled. If the function takes template arguments then this is easier since we know that the function can only be compiled if the template arguments are known, which can only occur if the function is actually called somewhere in the code. If the function takes no template arguments then it appears it may be compiled whenever the class is instantiated depending on compiler, although I don't know why my version of static assert (written to be compatible with C++98) works while C++11's static_assert does not. It would be reasonable in this case for you to simply replace my static assert line with

throw invalid_operation("Cannot use a SpecialMatrix to index another array" ADEPT_EXCEPTION_LOCATION);

i.e. to replace the compile-time check with a run-time check.

Servomex-ivisser commented 3 months ago

Yeah, I get it now. The build process should only assert if the code is actually reachable. In our case SpecialMatrix::value_with_len_() it is not reachable and we should not be statically asserting.

Your static assert works, and I would not suggest you change it. But just FYI, static_assert() got fixed in gcc 13 to do what yours does. See this MRE on Compiler Explorer. (drop the gcc version down to 12.3 to see the original problem reappear)

Purely in theory, you could do away with yours and replace it with static_assert(). But that puts a requirement on the user to update their toolchain to gcc 13 or later.

I accept your suggestion to replace those static assertions that offend the IAR compiler with a runtime assertion or exception instead.

Servomex-ivisser commented 3 months ago

Afterthought.... Seeing as SpecialMatrix::value_with_len_() is not a virtual function, completely removing it is a form of static assertion too. If it were to be erroneously called we'd have a compiler error, albeit a different one.