#545 bool size is mismatched on C++

[pe^cjo]

src/include/ngspice/bool.h defines bool as int when in C mode, which has a size of 4 or 8. In C++ bool is just the standard one which has a size of 1.

There are several solutions possible. One would be to use BOOL everywhere, forcing C++ to use an int as the C side currently does. Or to use stdbool.h which is compatible with C++ but would not be backwards ABI compatible since this changes the size of bool on the C side.

I ran into this when accessing the pvecvaluesall.vecs.is_complex from C++ which always returns false because the field address is off by 3 due to the size mismatch.

[Holger Vogt]

I do not cosider this as being a bug. It is a ngspice 'feature' since its inception.

Therefore I also do not consider changing the ngspice code base.

So what will be the hint for C++ users? We might find a place (manual, web site ...) to provide some info on this issue.

[pe^cjo]

I think the only change that is needed is in sharedspice.h booleans need to change from bool to BOOL. In C both bool and BOOL are typedefed to int, but in C++ bool is a built-in type that has the wrong size.

I think this change has zero impact on C since both types are int, and will fix the incorrect C++ behavior where bool is a different size than the C bool. Changing bool to BOOL is both API and ABI compatible in C.

The only effect is that the C++ behavior is not obviously wrong. I can't think of any case where an existing C++ user depends on behavior that simply does not work.

If maintaining broken C++ behavior is absolutely essential, the only workaround is to manually index the struct with pointer arithmetic. An ifndef was recently added to bool.h exactly because it is not possible to redefine bool in C++.

[Francesco Lannutti]

I'm a bit rusty, but why we don't do something like:

#ifdef C_link_mode
  #define bool int
#endif

In C++ link mode bool will remain bool.
There should be a macro to indicate C link mode, but I don't recall it now.

Fra

[pe^cjo]

Isn't that basically what is currently happening?
https://sourceforge.net/p/ngspice/ngspice/ci/master/tree/src/include/ngspice/bool.h#l3

//typedef unsigned char bool;
#ifndef __cplusplus
typedef int bool;
#endif

So in C mode bool is int, which could be 4 bytes on most system.
So the C code generates vecvalues with is_complex and is_scale as a 4 byte values.

When you access this data from C++, it assumes bool is 1 byte, so is_complex is at the wrong offset completely because is_scale is actually 4 bytes instead of 1.

There are two ways to go on this.

Make C bool one byte, as was done 11 years ago with the commented out line. So one could argue this bug wasn't there since it's inception, but introduced later. On the other hand, this changes the size of bool in a lot of existing code.

typedef unsigned char bool;

The other solution is to make bool 4 bytes in C++, so that it matches the current behavior. The problem here is that bool is a built-in C++ type that can't be redefined, I've been told. So the only way to make sharedspice.h compatible is to change the bool type to be int. Now in bool.h bool and BOOL are both typedefed as int, so changing bool to BOOL in sharedspice.h changes nothing for C. It changes from int to int. The only change is that for C++ it is now also interpreted as an int, matching the C behavior.

[Francesco Lannutti]

Ok!
This is the recap. Thanks for that, so I'm up to date! :)
However, I'm not getting why should access a C compiled code from C++. This leads to unpredictable results if the C code contains a lot of legacy.
If you compile everything is C++, you should not get (at least) this problem, because of the ifndef.

Said that, if we have to modify the code, I will prefer to adapt the C mode with the minimum impact, which means char = bool . We will also reduce the memory footprint by doing this, which is another good topic.
Anyway, a good testing has to be done, because I'm pretty sure there could be some code legacy that accesses the memory by bytes and thus will be impacted by this change.

Fra

[Holger Vogt]

It is about using only the header sharedspice.h in C++ code.

Internally ngspice uses typdefs from various bool to int everywhere, stemming from different (Partially ancient) sources. It might be advisable in the long run to intoduce a true bool type, but this does not have high proirity. In the short term I am willing to replace bool by BOOL in sharedspice.h and add a
typedef int BOOL;
In MSVC this typedef is already available (doubling it seems to be without problems). In Linux, Cygwin and macOS I will run some tests, because I am not sure if BOOL is available. The user should not need to resort to include/ngspice/ bool.h.

[pe^cjo]

In theory "extern C" should make C++ use C compatible conventions, but there are apparently some edge cases. I'm using ngspice with a C++ library, so using C is not an option. I agree compiling the whole of ngspice as C++ might avoid this particular problem, but will no doubt uncover others.

IMO changing bool to BOOL only in sharedspice.h is the more low-impact solution. That's 8 instances that change from int to int. As you say changing bool to be char, may uncover bugs in legacy code that lead to it being defined as int 11 years ago. But I'm fine with either solution.

Another solution that preserves ABSOLUTE backwards compatibility is to add a sharedspice.hpp header that uses BOOL. As said, the change is ABI compatible, so old code can continue to use sharedspice.h, while new C++ code can use sharedspice.hpp which has a C++ compatible definition where the boolean fields are int or BOOL. One could argue that this adds extra maintenance, but if you're of the position that any change to sharedspice.h is unacceptable for compatibility, the API is essentially set in stone, and adding a second C++ compatible version is no extra burden.

[pe^cjo]

Ah we were writing at the same time. Great!

[Holger Vogt]

A fix is uploaded to git branch master.

[pe^cjo]

Hm I wanted to test this fix, but the latest git version doesn't install headers for me using this package https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=ngspice-git
I'll debug that another time, but I assume it'll work.