ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
187 stars 31 forks source link

Migrate code to C99 #777

Open McDutchie opened 2 months ago

McDutchie commented 2 months ago

We now require C90, but the C99 standard is 25 years old. It's time. As discussed in https://github.com/ksh93/ksh/discussions/772#discussioncomment-10224924, moving to C99 can fix undefined behaviour (UB) that the ast and ksh code base is currently suffering from:

  1. Lots of uses of the struct hack in libast and ksh, for example, the final dolval/argval members of struct dolnod and struct argnod which are central to command arguments processing in ksh. It works in practice because C90 had no alternative and compiler implementers just made it work and allowed out-of-bounds array access regardless of what the standard said. But C99 does have a good construct for exactly that use case (flexible array members), so we should use it. There is no guarantee the struct hack will still work in practice 25 years from now. The change should be made very carefully as flexible array members have a size of zero, so the often-used sizeof of these structs will change.
  2. Undefined behaviour in many places with regards to float values and their typecasts to/from integer values, e.g., this typecast which has UB if the float value is infinite or is not a number (NaN), a scenario which can easily occur in practice. To fix issues like these we need fpclassify(3) and/or isnan(3), isinf(3), etc., which were all introduced in C99. The libast code does feature tests for these now, so we could implement the fixes conditionally, but it's 2024 and it's better just to require them at this point.
  3. More to be found?
McDutchie commented 2 months ago

Alternatively, we can keep C90 compatibility with the struct hack for now since it won't realistically be a problem anytime soon, if ever.

But problem 2, the undefined float behaviour, is actual breakage occurring right now. So at minimum we should modify the libast/features/float tests to error out if fpclassify(3) and friends are not present and working, and then use them to fix this. (Somewhat related issue: #771)

McDutchie commented 8 hours ago

To remember: since C99 can initialise struct members by name, localeconv.c can go back to using static initialisers instead of an initialiser function.