Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicate inline which newer gcc rejects #551

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@arpruss
Copy link
Contributor

arpruss commented Aug 4, 2018

Gcc 7 does not allow duplicate inline directives for one function. This commit makes the core compile under gcc 7.

@stevstrong

This comment has been minimized.

Copy link
Contributor

stevstrong commented Aug 5, 2018

Hm, I would rather remove the __always_inline directive, and just leave inline.
Can you eventually try whether you get the same result (code size) when you remove one or the other?

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 5, 2018

They are different compiler directives. I would assume that whoever wrote each part of the code would be making a decision whether they want to inline always or only inline when the compiler thinks it's a good idea. I think it's better to respect the original author's decision to force an inline with __always_inline rather than changing them all to optional inlines.

Checking for sameness of results would require recompiling with different compilers and multiple optimization levels.

@RickKimball

This comment has been minimized.

Copy link
Contributor

RickKimball commented Aug 5, 2018

I think I was the one who checked this stuff in to clean up some of gcc compiler warning in an earlier version of gcc. It used to complain if the inline was after the attribute. gcc used to suggest this way of using their __always_inline:

Originally gcc suggested this usage:
/* Prototype.  */
inline void foo (const char) __attribute__((always_inline));

so the original libmaple code would spew a warning.

now __always_inline in gcc now looks like this:

#if __GNUC_PREREQ__(3, 1) || (defined(__INTEL_COMPILER) && __INTEL_COMPILER >= 800)
#define	__always_inline	__inline__ __attribute__((__always_inline__))
#else
#define	__always_inline
#endif

We should make our fallback __always_inline define look like the new one. We should also get rid of the inline keywords in the code I added to make that earlier version of gcc stop complaining.

@RickKimball

This comment has been minimized.

Copy link
Contributor

RickKimball commented Aug 5, 2018

@arpruss actually, can you provide a sketch example where this is failing?

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 5, 2018

I think just a blank sketch might show the issue.

@rogerclarkmelbourne

This comment has been minimized.

Copy link
Owner

rogerclarkmelbourne commented Aug 6, 2018

My concern is that we break, or affect, the operation for anyone using the normal Arduino IDE, just to support people using the repo with other IDE's

Operation with the normal Arduino IDE will always take priority over other IDE's or build systems, because the vast majority of users, use the Arduino IDE.

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 6, 2018

I would think __always_inline by itself will work for all gcc versions, no?

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 9, 2018

I checked that replacing __always_inline with inline increases code size on p08_DigitalHourglass with the default compiler.

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 9, 2018

@RickKimball:

So, is your suggestion that we replace "inline __always_inline" with just plain "__always_inline", but update the __always_inline macro in libmaple_types.h to the new one, and this will work on gcc 3.0 and below but without inlining, and will work fully with inlining on gcc 3.1 and above (which is presumably what everyone uses)?

I just updated this PR to do that.

@RickKimball

This comment has been minimized.

Copy link
Contributor

RickKimball commented Aug 9, 2018

Looking at the code, I'd be surprised if a lot of those functions were ever not inlined

static __always_inline void nvic_globalirq_disable() {
    asm volatile("cpsid i");
}

did you try that with just inline?

@rogerclarkmelbourne

This comment has been minimized.

Copy link
Owner

rogerclarkmelbourne commented Aug 19, 2018

Do we still need to have this change. ?

I get the feeling that the reason for it was to use GCC 7 to resolve issues in the USB Lib, where unused code was being pulled in.

As most people will only be using the GCC version that comes with the IDE, I think its quite risky to be making this change for no gain on the version of GCC most people are using

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 19, 2018

Yeah, I can do without it. But one day Arduino may update the compiler and then it'll be an issue.

@rogerclarkmelbourne

This comment has been minimized.

Copy link
Owner

rogerclarkmelbourne commented Aug 19, 2018

OK.
I'll close this, and refer to the discussion again in the future if necessary.

PS. Are you doing a PR for the changes to the USB Lib ?

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 19, 2018

Roger: If you want, you can put the 0.91 release of the library into the core ( https://github.com/arpruss/USBHID_stm32f1/releases/tag/0.91 ). I often screw up when I generate PRs, so it might be better if you did it. (N.B.: If you want to put the files from the release on top of the old ones, you'll also have to delete the old BootKeyboard.cpp file as it's no longer needed.)

@rogerclarkmelbourne

This comment has been minimized.

Copy link
Owner

rogerclarkmelbourne commented Aug 19, 2018

Can I just delete my local copy of the library and replace with the ones in https://github.com/arpruss/USBHID_stm32f1/releases/tag/0.91 ?

Git will then handle things, albeit as if I changed the code

@arpruss

This comment has been minimized.

Copy link
Contributor Author

arpruss commented Aug 20, 2018

Sure. Just make sure that BootKeyboard.cpp gets deleted.

@rogerclarkmelbourne

This comment has been minimized.

Copy link
Owner

rogerclarkmelbourne commented Aug 20, 2018

OK.

I'll delete the entire contents of the folder

@majenkotech

This comment has been minimized.

Copy link

majenkotech commented Nov 30, 2018

UECIDE switched to GCC 7 for all ARM compilation recently. This was to facilitate having a single unified ARM compiler for all the myriad of ARM-based boards out there.

Now I'm porting these cores to UECIDE and it's failing because of this bug. For now I can manually patch, but it would be good to get some official resolution for this problem. Really it should be one of the other: either you let the compiler decide to inline (inline) or you force inline (__always_inline) but not both. Historically one has overridden the other, but no longer. Really it does need officially fixing sometime soon. Make your mind up what kind of inlining you actually want. Don't try and use both.

@stevstrong

This comment has been minimized.

Copy link
Contributor

stevstrong commented Dec 7, 2018

Wouldn't be better to move the inline function from exti.c to a header file (exti.h)?
The inline directive for functions placed in header files will always result in including those functions inline.

@pinchies

This comment has been minimized.

Copy link

pinchies commented Feb 24, 2019

I think I'm running into this issue using Platformio on a Mac?

My compiler is Clang - could that be the problem?

clang --version
Apple LLVM version 10.0.0 (clang-1000.11.45.5)
Target: x86_64-apple-darwin17.7.0
Thread model: posix

I'm working on adding another 3D printer to the Marlin firmware project, with the same CPU as the MKS Robin, but I can't get the MKS Robin example config in the current marlin 2.0 build to compile under any version of ststm32. I've tried all of the ST STM32 platform versions that support this CPU

(latest git, 5.1.0, 5.0.0, 4.6.0, 4.5.0) but I get the same error for them all - errors about a bunch of duplicate inline errors.

e.g.
platform = https://github.com/platformio/platform-ststm32.git

/.platformio/packages/framework-arduinoststm32-maple/STM32F1/system/libmaple/stm32f1/include/series/gpio.h:489:15: error: duplicate 'inline'

From what I can tell, the framework-arduinoststm32-maple is a nice clean separation of the community STM32duino libraries out of the previous framework framework-arduinoststm32, which contained both. But I don't think that is the problem, as I get the same errors about inline errors going back further with those older ststm32 versions.

@pinchies

This comment has been minimized.

Copy link

pinchies commented Feb 24, 2019

To confirm: I built this on a PC and the problem was gone.... so looks like a compiler related issue. Would be nice if there was a fix, but I think that's outside of my skill level to attempt.

@qu1ck

This comment has been minimized.

Copy link

qu1ck commented Feb 24, 2019

I am running into this as well. My project started failing build on travis-ci, I'm compiling using platformio.

https://travis-ci.com/openscopeproject/HP34401a-OLED-FW/jobs/180068697

@pinchies

This comment has been minimized.

Copy link

pinchies commented Feb 24, 2019

@qu1ck - platformio summons your compiler on your system, so the compiler it uses will depend on the platform you run platformio on. When I run platformio on a PC (hosted under Visual Code), I don't get those errors.

@qu1ck

This comment has been minimized.

Copy link

qu1ck commented Feb 24, 2019

@pinchies I understand that. I'm just adding a data point to show that this problem will only become more widespread and needs fixing. Platformio will just pull the fix into it's own ststm32 'core' repo eventually.

I doubt this is an either/or situation but if the choice is between code being completely broken on new compiler and throwing a warning on the old one I would rather live with the latter.

@rogerclarkmelbourne

This comment has been minimized.

Copy link
Owner

rogerclarkmelbourne commented Feb 24, 2019

@stevstrong

I don't think @arpruss responded to your question.

I don't want to attempt to merge this until they respond

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.