int (also literals as it seems) are signed by default #1575

smoe · 2022-02-06T14:47:47Z

These patches eliminate an error found by cppcheck:
src/hal/drivers/opto_ac5.c:432:13: error: Signed integer overflow for expression '1<<(31-i)'. [integerOverflow]
mask=1<<(31-i);

This is just a pilot PR to test my thought to have cppcheck integral to the continuous integration (#1574).

Many thanks!
Steffen

SebKuzminsky · 2022-02-06T21:47:16Z

I'm in favor of the syntax change in this commit, ie of moving the variable definition from the function scope into the smaller scope of the for loop, with definition in the for loop itself. However, this conflicts with a bug in our RTAI debs for Buster that @dngarrett pointed out recently:

3768a3c

The ideal way to fix this would be to fix that version of our RTAI packages so that we can use a modern (well, a less ancient) C standard. I think @andypugh or @NTULINUX prepared those RTAI debs, do you have input for us on this topic?

smoe · 2022-02-09T11:02:45Z

@SebKuzminsky Together with the additional instance if a (signed int) << shifted I have also moved the initialization of i and j back up to the start of the function.

@andypugh The effect of compiler-whatever options on RTAI is interesting, though. Not that I would understand this. Link-time optimization would be close to my heart since this reduces the memory footprint which I could imagine to reduce the jitter.

NTULINUX · 2022-02-09T22:54:20Z

The RTAI CFLAGS outside of LIBM (musl math libm math library) are:

-O2 -march=x86-64 -fomit-frame-pointer -pipe

-fomit-frame-pointer helps decrease code size, maybe increase performance a tad, but makes debugging a bit more difficult. RTAI is pretty much hit-or-miss anyway, and frame pointers won't help you lol.

-pipe allows for faster compilation, does not affect generated code at all.

For the math library, it is the above CFLAGS but with these appended:

-D__IN_RTAI__ -mcmodel=kernel -nostdinc -ffreestanding -fno-pic -fno-builtin -fno-common -fno-unwind-tables -fno-asynchronous-unwind-tables -frounding-math -mpreferred-stack-boundary=4 -mstackrealign -mfpmath=sse -msse -msse2

-D__IN_RTAI__ pertains to autoconf/Kconfig stuff, don't worry about that really:

dnl CAUTION: We need to have the CONFIG_RTAI_XX symbols always defined when
dnl the configuration header is read, but we want the Autoconf-produced
dnl symbols to be defined only when compiling RTAI. This way, we won't
dnl pollute the namespace with the latter when our configuration header is
dnl indirectly included by a client application. To achieve this, we ask
dnl autoheader to produce the following header structure:
dnl #define CONFIG_XX
dnl #define CONFIG_XX ...
dnl #ifdef __IN_RTAI__
dnl <Autoconf-defined symbols>
dnl #endif /* __IN_RTAI__ */
dnl This is quite a hack since we have to rely on the fact that
dnl all Autoconf-generated symbols are lexicographically sorted
dnl after CONFIG_RTAI_XX ones, but, well...

-mcmodel=kernel is generate code for kernel model.

-fno-pic is because you cannot build position independent code (PIC) in kernel space. PIC is a security feature, but does not work in cases such as the Linux kernel, or certain assembly instructions that is not PIC-friendly.

https://en.wikipedia.org/wiki/Position-independent_code

-nostdinc -ffreestanding -fno-builtin -fno-common These basically just mean "Don't use anything, from anyone, outside of this scope. Don't use compiler built-in functions, don't grab things from anywhere else, this is a free standing environment, don't even try looking outside the box."

-fno-unwind-tables -fno-asynchronous-unwind-tables are to reduce generated code size, may improve performance. Again, makes debugging more difficult but these probably won't help you anyway.

-frounding-math -mpreferred-stack-boundary=4 -mstackrealign -mfpmath=sse -msse -msse2 These all work together to satisfy the build and runtime of floating point instructions. -mstackrealign basically makes sure that RTAI modules are aligned to kernel space; helps keep things in order. mfpmath=sse -msse -msse2 are x86 extensions which should be used to aid in floating point precision.

https://en.wikipedia.org/wiki/Streaming_SIMD_Extensions

SebKuzminsky · 2022-02-10T03:17:37Z

@NTULINUX Thanks for that run-down.

Do you have any idea about the source of the -std=gnu89 that @dngarrett reports here? 3768a3c#commitcomment-65892820
It seems to somehow make it into LinuxCNC's CFLAGS when building on RTAI.

The ancient Wheezy RTAI does not have this problem, it builds LinuxCNC with -std=gnu99 which accepts this syntax, eg: http://buildbot.linuxcnc.org/buildbot-admin/builders/1401.rip-wheezy-rtai-i386/builds/6346/steps/compile/logs/stdio

NTULINUX · 2022-02-13T06:23:32Z

My guess is:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=51b97e354ba9fce1890cf38ecc754aa49677fc89

RTAI legacy (RTAI 4 releases) goes up to only the 3.16 kernel series, LinuxCNC's ancient RTAI kernel version is 3.4.55 I believe. RTAI currently only supports the 4.19 series, and the above commit, explicitly adding -std=gnu89 to the kernel's CFLAGS was added to 3.18-rc4, and still exists today in the latest 4.19 kernels.

Cheers!

smoe · 2022-03-20T20:22:24Z

This (too long) video https://www.youtube.com/watch?v=-G1FuEQqxVI just informed me that apparently the Linux kernel is about to upgrade to C99 .

andypugh · 2022-03-30T22:34:39Z

Where are we with this? Master/2.9 is currently not targeting any OS older than Buster, so we may have the opportunity to drop some legacy limitations.
I am still committed to keeping an RTAI kernel available and supported, but now that LinuxCNC is included in (future) Debian in the preempt-rt version we probably should not consider RTAI-specific issues to be blocking.

smoe · 2022-03-30T22:40:45Z

I think this is fine. I had undone my compiler-incompatibility as mentioned in #1575 (comment) .

petterreinholdtsen · 2022-07-11T08:54:18Z

src/hal/drivers/hal_gm.c

    	card	*pCard = device->pCard;
-    	int		i;
 	hal_u32_t temp;
-
+	unsigned int i;


Why do you change location of the declaration?

I wish you had asked me a few months ago. Looking at it now I feel like i may have ordered the variables by their "localness", i.e. these are declared function-wide but used over different regions of the code and i I may have perceived as the most local.

petterreinholdtsen · 2022-07-11T13:48:00Z

[Steffen Möller]

I wish you had asked me a few months ago. Looking at it now I feel like i may have ordered the variables by their "localness", i.e. these are declared function-wide but used over different regions of the code and i I may have perceived as the most local.

Is the idea that the compiler can not figure this out on its own? Tried to consult docs/src/code/style-guide.adoc to see if it had something useful on the topic, but alas.

…

-- Happy hacking Petter Reinholdtsen

smoe · 2022-07-11T14:51:35Z

I likely have edited it back in after having tried this within the for loop itself and forgot where I got it from. While I tend to think this is an improvement as it is, this is not meant to have an effect on the compiler but solely beween us devs.

These patches eliminate an error found by cppcheck: src/hal/drivers/opto_ac5.c:432:13: error: Signed integer overflow for expression '1<<(31-i)'. [integerOverflow] mask=1<<(31-i);

@seb

Also reacted to @seb's comments in PR 1575.

smoe · 2022-07-21T17:17:09Z

Brought the declaration of local variables back into the original order as suggested in #1575 (comment)

petterreinholdtsen · 2022-07-24T09:00:42Z

The updated pull request look good to me, even if it fail to clean up some already existing white space issues (reported as red in 'git diff') against marster in the lines it touces. The code in question compile without any compiler warnings, and all tests succeed.

…

-- Happy hacking Petter Reinholdtsen

petterreinholdtsen · 2022-08-13T13:05:19Z

The change look good to me. The current failing build tests are due to a transient error on github, and not related to the patch. I guess a rebase and rebuild will show success.

jepler · 2022-08-17T01:51:36Z

a matter of style "1u" also works when you want an unsigned literal value.

smoe mentioned this pull request Feb 14, 2022

Formatting and linting the codebase machinekit/machinekit-hal#374

Open

3 tasks

petterreinholdtsen reviewed Jul 11, 2022

View reviewed changes

int (also literals as it seems) are signed by default

3c6b533

These patches eliminate an error found by cppcheck: src/hal/drivers/opto_ac5.c:432:13: error: Signed integer overflow for expression '1<<(31-i)'. [integerOverflow] mask=1<<(31-i);

smoe force-pushed the cppcheck_fixes branch from 59cb9ef to 0c3e32e Compare July 21, 2022 17:11

Found another instance if signed ints <<-shifted

0f32d7e

Also reacted to @seb's comments in PR 1575.

smoe force-pushed the cppcheck_fixes branch from 0c3e32e to 0f32d7e Compare July 21, 2022 17:14

jepler merged commit 3450be8 into LinuxCNC:master Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

int (also literals as it seems) are signed by default #1575

int (also literals as it seems) are signed by default #1575

smoe commented Feb 6, 2022

SebKuzminsky commented Feb 6, 2022

smoe commented Feb 9, 2022

NTULINUX commented Feb 9, 2022 •

edited

Loading

SebKuzminsky commented Feb 10, 2022

NTULINUX commented Feb 13, 2022 •

edited

Loading

smoe commented Mar 20, 2022

andypugh commented Mar 30, 2022

smoe commented Mar 30, 2022

petterreinholdtsen Jul 11, 2022

smoe Jul 11, 2022

petterreinholdtsen commented Jul 11, 2022 via email

smoe commented Jul 11, 2022

smoe commented Jul 21, 2022 •

edited

Loading

petterreinholdtsen commented Jul 24, 2022 via email

petterreinholdtsen commented Aug 13, 2022

jepler commented Aug 17, 2022

int (also literals as it seems) are signed by default #1575

int (also literals as it seems) are signed by default #1575

Conversation

smoe commented Feb 6, 2022

SebKuzminsky commented Feb 6, 2022

smoe commented Feb 9, 2022

NTULINUX commented Feb 9, 2022 • edited Loading

SebKuzminsky commented Feb 10, 2022

NTULINUX commented Feb 13, 2022 • edited Loading

smoe commented Mar 20, 2022

andypugh commented Mar 30, 2022

smoe commented Mar 30, 2022

petterreinholdtsen Jul 11, 2022

Choose a reason for hiding this comment

smoe Jul 11, 2022

Choose a reason for hiding this comment

petterreinholdtsen commented Jul 11, 2022 via email

smoe commented Jul 11, 2022

smoe commented Jul 21, 2022 • edited Loading

petterreinholdtsen commented Jul 24, 2022 via email

petterreinholdtsen commented Aug 13, 2022

jepler commented Aug 17, 2022

NTULINUX commented Feb 9, 2022 •

edited

Loading

NTULINUX commented Feb 13, 2022 •

edited

Loading

smoe commented Jul 21, 2022 •

edited

Loading