ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

[cmsis_gcc.h] nested extern declaration #617

Open salkinium opened 5 years ago

salkinium commented 5 years ago

When compiling with -Wnested-externs, these warnings appear:

modm/ext/cmsis/core/cmsis_gcc.h: In function '__cmsis_start':
modm/ext/cmsis/core/cmsis_gcc.h:133:15: warning: nested extern declaration of '_start' [-Wnested-externs]
   extern void _start(void) __NO_RETURN;
               ^~~~~~
modm/ext/cmsis/core/cmsis_gcc.h:146:31: warning: nested extern declaration of '__copy_table_start__' [-Wnested-externs]
   extern const __copy_table_t __copy_table_start__;
                               ^~~~~~~~~~~~~~~~~~~~
modm/ext/cmsis/core/cmsis_gcc.h:147:31: warning: nested extern declaration of '__copy_table_end__' [-Wnested-externs]
   extern const __copy_table_t __copy_table_end__;
                               ^~~~~~~~~~~~~~~~~~
modm/ext/cmsis/core/cmsis_gcc.h:148:31: warning: nested extern declaration of '__zero_table_start__' [-Wnested-externs]
   extern const __zero_table_t __zero_table_start__;
                               ^~~~~~~~~~~~~~~~~~~~
modm/ext/cmsis/core/cmsis_gcc.h:149:31: warning: nested extern declaration of '__zero_table_end__' [-Wnested-externs]
   extern const __zero_table_t __zero_table_end__;
                               ^~~~~~~~~~~~~~~~~~

I'm currently working around this by #define __PROGRAM_START 1, but that's pretty brute force.

salkinium commented 5 years ago

Uses the latest version of these files.

JonatanAntoni commented 5 years ago

Hi @salkinium,

thanks for pointing this out. These warnings are caused by the reworked C startup code (compiler agnostic). I'll check what's behind and how we can change it.

As long as you are using the former startup routines (compiler specific) you are fine with defining __PROGRAM_START yourself. The new startup code would fail if __PROGRAM_START doesn't name a proper library (or application) start routine.

Cheers, Jonatan

salkinium commented 5 years ago

Yeah, redefining __PROGRAM_STARTis very hacky and fragile. Unfortunately, since it‘s a header file, it‘s a bit difficult to not leak the extern variables everywhere… So I understand why they were extern declared inside the function.

JonatanAntoni commented 5 years ago

Hi @salkinium,

I checked with our GCC experts and the code should just be fine. Do you have any concerns if we simply suppress that warning in that location? I.e. by adding #pragma GCC diagnostic ignored "-Wnested-externs"?

Cheers, Jonatan

salkinium commented 5 years ago

No, suppressing the warning locally is fine for me.

JonatanAntoni commented 5 years ago

May I ask you for what reason you enabled that warning? There seems nothing wrong with using this kind of extern declaration. Hence I am now struggling between adding the pragmas to suppress the warning locally versus keeping the code clean. As I don't see any problems with that code I tend to conclude: That warning should not be used.

salkinium commented 5 years ago

cc @dergraaf Why was -Wnested-externs added?

Is there a complete list of compile flags to use to compile your code? Then I can check what the differences are and adapt our flags to that. Or since we only include the header file once, I can also wrap the include in the #pragma.

salkinium commented 5 years ago

Or since we only include the header file once, I can also wrap the include in the #pragma.

I've solved it this way. I think this is best since -Wnested-externs is a style choice, not a technical one.

Thanks for you help!

zejiang0jason commented 4 years ago

Hi @JonatanAntoni , I got the similar error while including the "arm_math.h" in *.cpp file, the gcc error is:

const __cmsis_start()::__copy_table_t __copy_table_start__', declared using local type 'const __cmsis_start()::__copy_table_t', is used but never defined [-fpermissive]
   extern const __copy_table_t __copy_table_start__;

The flag -fpermissive could convert the error to warning, but this is not a good solution for our customers. What is your suggestions for this? Is there a plan to update for this?

By the way, when I changed the cmsis_gcc.h, moving

  extern void _start(void) __NO_RETURN;

  typedef struct {
    uint32_t const* src;
    uint32_t* dest;
    uint32_t  wlen;
  } __copy_table_t;

  typedef struct {
    uint32_t* dest;
    uint32_t  wlen;
  } __zero_table_t;

  extern const __copy_table_t __copy_table_start__;
  extern const __copy_table_t __copy_table_end__;
  extern const __zero_table_t __zero_table_start__;
  extern const __zero_table_t __zero_table_end__;

out of the function __cmsis_start, then the error disappears.

JonatanAntoni commented 4 years ago

Hi @zejiangfish,

I guess this happens because the symbols in question (e.g. __copy_table_start__) are linker generated. Hence the compiler doesn't see the definition.

We could of course move the type defines and extern declarations into global scope. I added them to local scope in order not to clutter the global scope.

Cheers, Jonatan

zejiang0jason commented 4 years ago

Hi @JonatanAntoni ,

Thank you very much for analyze, is there plan for this error? Or some suggestions? thanks.

noxuz commented 4 years ago

Facing the same problem here, moving the type defines out of the function clears the compiler error.

error: 'const __cmsis_start()::__copy_table_t __copy_table_start__', declared using local type 'const __cmsis_start()::__copy_table_t', is used but never defined [-fpermissive]

JonatanAntoni commented 4 years ago

Hi @noxuz,

Thanks for getting in touch. I had clarified with our Compiler experts back in June last year that these nested declarations should just be fine.

Why is this causing errors for you?

Cheers, Jonatan

noxuz commented 4 years ago

@JonatanAntoni Thank you for the follow-up.

I was trying to compile a project using directly the CMSIS library from this repo, although I was not using that particular function, the compiler did popped the same error from the top of this post.

I'm currently working with NXP mcu's, and I ended up using the CMSIS-like header that its provided from within the IDE for it. You can take a look at it in this Link.

yan12125 commented 4 years ago

I got the same error as @zejiangfish, and I managed to get a patch with only the struct declarations moved: https://github.com/EMCLab-Sinica/ARM-CMSIS_5/commit/f37a11415a37df993e990b8e8a1d5f8b05edb91b. extern symbol declarations are kept local. That should reduce the risk of symbol conflicts.

JonatanAntoni commented 4 years ago

@yan12125,

which version of GCC are you using? As stated above, the nested declarations should be just fine from C standard point of view. Is this caused by a shortcoming in certain compiler versions?

Cheers, Jonatan

yan12125 commented 4 years ago

I'm using GCC 10.1. I guess GCC does not like it as I'm compiling C++ code. I remember CMSIS worked fine with one of my another project with only C code.

yan12125 commented 4 years ago

Apologies - it turns out the problem is from dependency management of our projects. The "declared using local type" error appears only with CMSIS 5.6.0. CMSIS 5.7.0 works fine with C++ source files (our code base uses C++ 11, if that is relevant).

As a side note, the key change in CMSIS between 5.6.0 and 5.7.0 appears to be https://github.com/ARM-software/CMSIS_5/commit/3b2a0ee23428eb1f230672d9061da4e6b7afa3db#diff-b9d8110b101a50848c56c0c9c90fc936. As cmsis_gcc.h, which is included from cmsis_compiler.h, is protected by the extern "C" block since that commit, GCC no longer complains about local types.

CheMax-Tag commented 3 years ago

Hello, today I meet this error too. I use Segger Embedded Studio with GCC compiler (standarts: c++17, c17). For fast "bugFix", I add missing "extern C" guard in "cmsis_gcc.h" file.

But patching package files supplied by the IDE developer is bad form. Therefore, I cover all the including of this file with guards. Maybe you should add guards "extern C" to all header files?

JonatanAntoni commented 3 years ago

Hi @CheMax-Tag,

The extern C declaration is in all the core_cmX.h files. So including the cmsis_gcc.h file the regular way (through the device header including the required core header) should not cause any issues.

Do you have specific requirements why you'd want to include internal stuff of CMSIS directly from user application?

Cheers, Jonatan

CheMax-Tag commented 3 years ago

Hi @JonatanAntoni ,

I include this file to access assembly wrappers, such as SEV(); WFE(); __BKPT() and other.

Since the file in which I make the connection is used for different MCUs, I wanted to make a universal solution. Therefore, I connected a "universal" file.

For the test, I tried to include "core_cm4.h", but got another error: IRQn_Type, defined in "stm32l4r9xx.h" is not visible in "core_cm4.h".

Regards, Max

ilg-ul commented 3 years ago

I had a similar experience.

Anyway, it is good practice for each header file to include the extern "C" braces.

JonatanAntoni commented 3 years ago

I agree, if done properly the extern "C" should not harm, either. May one of you find the time to raise the required changes as a PR? Would be quite helpful.

Thanks, Jonatan

CheMax-Tag commented 3 years ago

@JonatanAntoni , Ok. I try do it.

dzid26 commented 3 years ago

Hi @CheMax-Tag,

The extern C declaration is in all the core_cmX.h files. So including the cmsis_gcc.h file the regular way (through the device header including the required core header) should not cause any issues.

In my case I am including cmsis_gcc regular way via core_cm4.h, and with gcc the error doesn't show up.

But the project I am using was configured to Clang 12 and I was getting following error:

In file included from ../../board/inc/stm32f4xx.h:189:
In file included from ../../board/inc/stm32f413xx.h:197:
In file included from ../../board/inc/core_cm4.h:162:
In file included from ../../board/inc/cmsis_compiler.h:54:
../../board/inc/cmsis_gcc.h:135:17: error: unsupported: anonymous type given name for linkage purposes by typedef declaration after its linkage was computed; add a tag name here to establish linkage prior to definition
  typedef struct {
                ^
../../board/inc/cmsis_gcc.h:139:5: note: type is given name '__copy_table_t' for linkage purposes by this typedef declaration
  } __copy_table_t;
    ^
../../board/inc/cmsis_gcc.h:141:17: error: unsupported: anonymous type given name for linkage purposes by typedef declaration after its linkage was computed; add a tag name here to establish linkage prior to definition
  typedef struct {
                ^
../../board/inc/cmsis_gcc.h:144:5: note: type is given name '__zero_table_t' for linkage purposes by this typedef declaration
  } __zero_table_t;
    ^

Should Clang use a different header?

Interestingly this ,global define fixes it even for Clang.

JonatanAntoni commented 3 years ago

Hi @dzid26,

I can only speculate because using vanilla Clang is currently not in scope.

The "problem" might be the declaration of extern variables with non-external types. Strictly speaking no extern module would ever be able to define a variable with that function-local type.

In this specific case the extern variables are linker defined trough the used linker script. The type's memory layout is respected without actually using the type delaration from the c module, of course.

The compiler cannot know this. By moving the type declaration to global scope the warning disappears. A global type could be used from an external module to define the variable. If it is actually used doesn't matter, here.

The reason for having the type declaration local to the function is to prevent cluttering global scope. In fact, this type should not be required/used in any other place.

Can you try to modify the code as proposed by the error message, please? I.e.

[..] add a tag name here to establish linkage [..]

so the code should become something like

  typedef struct copy_table_t {
    uint32_t const* src;
    uint32_t* dest;
    uint32_t  wlen;
  } __copy_table_t;

Does this solve the issue for you? If yes, would you mind to raise this change as a pull-request?`

Thanks, Jonatan

puzrin commented 3 years ago

In .cpp file, if i include like this:

extern "C" {
    #include "arm_math.h"
}

them errors disappear.

syousufimam commented 2 years ago

Hi @JonatanAntoni , I got the similar error while including the "arm_math.h" in *.cpp file, the gcc error is:

const __cmsis_start()::__copy_table_t __copy_table_start__', declared using local type 'const __cmsis_start()::__copy_table_t', is used but never defined [-fpermissive]
   extern const __copy_table_t __copy_table_start__;

The flag -fpermissive could convert the error to warning, but this is not a good solution for our customers. What is your suggestions for this? Is there a plan to update for this?

By the way, when I changed the cmsis_gcc.h, moving

  extern void _start(void) __NO_RETURN;

  typedef struct {
    uint32_t const* src;
    uint32_t* dest;
    uint32_t  wlen;
  } __copy_table_t;

  typedef struct {
    uint32_t* dest;
    uint32_t  wlen;
  } __zero_table_t;

  extern const __copy_table_t __copy_table_start__;
  extern const __copy_table_t __copy_table_end__;
  extern const __zero_table_t __zero_table_start__;
  extern const __zero_table_t __zero_table_end__;

out of the function __cmsis_start, then the error disappears.

Hi @zejiang0jason

Thanks for your suggestions. Finally it worked.

 extern void _start(void) __NO_RETURN;

  typedef struct {
    uint32_t const* src;
    uint32_t* dest;
    uint32_t  wlen;
  } __copy_table_t;

  typedef struct {
    uint32_t* dest;
    uint32_t  wlen;
  } __zero_table_t;

  extern const __copy_table_t __copy_table_start__;
  extern const __copy_table_t __copy_table_end__;
  extern const __zero_table_t __zero_table_start__;
  extern const __zero_table_t __zero_table_end__;

__STATIC_FORCEINLINE __NO_RETURN void __cmsis_start(void)
{

  for (__copy_table_t const* pTable = &__copy_table_start__; pTable < &__copy_table_end__; ++pTable) {
    for(uint32_t i=0u; i<pTable->wlen; ++i) {
      pTable->dest[i] = pTable->src[i];
    }
  }

  for (__zero_table_t const* pTable = &__zero_table_start__; pTable < &__zero_table_end__; ++pTable) {
    for(uint32_t i=0u; i<pTable->wlen; ++i) {
      pTable->dest[i] = 0u;
    }
  }

  _start();
}
Cromagnon31 commented 2 years ago

Hi,

I encounter same issue. The main reason If I include cmsis_compiler.h directly it's to access cortex_m macros/types/inlines for sources common to different cores (m0, m0+, m3, m4...). There is an other ways to correct the issue. Simply encapsulate this code with an extern "C", since this code is only workable in "C". Extern "C" statement exists for that case.

But I also think that encapsulating #include files with extern "C" statement like in core_cm?.h files is definitely not a good idea and may have unexpected effects for C++ development. We are at the age where Linux kernel is migrating to C++ !

Olivier