Skip to content

Introduce stack definition macros #392

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

Merged
merged 8 commits into from
Jun 9, 2017

Conversation

andrewboie
Copy link
Contributor

We need a good method for defining stack memory regions. Right now the typical way to do it is with __stack __noinit. We need something more generic so that we can have fine, architecture-specific control on how the stacks are declared.

This patch series replaces all instances of __stack with K_THREAD_STACK_DECLARE() and related macros.

It is expected that arches will implement their own versions of these macros to implement whatever behind-the-scenes magic is necessary to set up these stacks for MMU or MPU-based memory protection.

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of use of __DEPRECATED_MACRO in the commit message.

@andrewboie
Copy link
Contributor Author

Whoops, git ate the line that started with #define in the commit message. Should be fixed now.

@@ -0,0 +1,119 @@
/*
* Copyright (c) 2016 Intel Corporation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deleted, mis-type in my text editor

@@ -98,7 +98,8 @@
/* Indicate that an array will be used for stack space. */

#if !defined(_ASMLANGUAGE)
#define __stack __aligned(STACK_ALIGN)
/* don't use this anymore, use K_DECLARE_STACK instead. Remove for 1.11 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a macro for version checking? Might be useful for future deprecations:

#if ZEPHYR_CHECK_VERSION(1,11)
#error Remove definition for __stack
#endif
``

(glib/gtk has a similar macro, and it's pretty useful.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we do, similar to the linux kernel:

#if ZEPHYR_VERSION_CODE <= ZEPHYR_VERSION(1,8,0)
foo
#else
bar
#endif

Copy link
Contributor Author

@andrewboie andrewboie Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anas, this doesn't work either, the preprocessor does not expand ZEPHYR_VERSION the way you expect:


/projects/zephyr2/include/toolchain/common.h:102:43: error: missing binary operator before token "("
 #if (ZEPHYR_VERSION_CODE <= ZEPHYR_VERSION(1,11,0))
                                           ^
/projects/zephyr2/include/toolchain/common.h:100:0: error: unterminated #if
 #if !defined(_ASMLANGUAGE)

Also, as noted below, what Leandro is proposing is not actually possible with the preprocessor.

I ask we leave this macro as-is and focus on the memory protection features in this patch set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to include version.h if I am not mistaken.

/* Be *very* careful with this, you cannot filter out with -wno-deprecated,
* which has implications for -Werror
*/
#define __DEPRECATED_MACRO _Pragma("GCC warning \"Macro is deprecated\"")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggested macro can even be used like so:

#define __DEPRECATED_MACRO(cond)  __DEPRECATED_MACRO_##cond
#define __DEPRECATED_MACRO_0 _Pragma("GCC warning \"Macro is deprecated\"")
#define __DEPRECATED_MACRO_1 _Pragma("GCC error \"Gardening required on this macro\"")

#define __DEPRECATE_MACRO_ON(maj, min) \
   __DEPRECATED_MACRO(ZEPHYR_VERSION_CHECK(maj,min))

And we can then use __DEPRECATE_MACRO_ON(1,11), ensuring a compile error will happen. (ZEPHYR_VERSION_CHECK() should evaluate to 0 or 1 given maj/min).

Copy link
Contributor Author

@andrewboie andrewboie Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you're proposing here isn't possible with the C preprocessor, you can't implement ZEPHYR_VERSION_CHECK(maj, min) in this way, there is no way to have a #define expand to an #if condition.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@cvinayak cvinayak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stack related macros look good to me.

Andrew Boie added 2 commits June 9, 2017 08:23
Add a macro which signals to the compiler that use of the macro is
deprecated.

Example:

  #define FOO __DEPRECATED_MACRO bar

Defines FOO to 'bar' but emits a warning if used in code.

Cannot filter out with -Wno-deprecated, so be careful with -Werror.

Signed-off-by: Andrew Boie <[email protected]>
The main thread was doing nothing but spawning another thread to perform
the test. Delete the alternate thread, and just do the test on the main
thread, adjusting stack size and priority as necessary.

Issue: ZEP-2236
Signed-off-by: Andrew Boie <[email protected]>
Andrew Boie added 3 commits June 9, 2017 12:06
The existing __stack decorator is not flexible enough for upcoming
thread stack memory protection scenarios. Wrap the entire thing in
a declaration macro abstraction instead, which can be implemented
on a per-arch or per-SOC basis.

Issue: ZEP-2185
Signed-off-by: Andrew Boie <[email protected]>
@nashif nashif merged commit 680ca8c into zephyrproject-rtos:master Jun 9, 2017
@andrewboie andrewboie deleted the stack-macros branch July 26, 2017 07:47
nagineni pushed a commit to nagineni/zephyr that referenced this pull request Nov 20, 2017
Signed-off-by: cuiyanx <[email protected]>

Fixed whitespace issues. Two test failures on A101 are by design but will
be addressed with redesign relatively soon.

Signed-off-by: Geoff Gustafson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants