xiaoxiang781216 commented on a change in pull request #1369:
URL: https://github.com/apache/incubator-nuttx/pull/1369#discussion_r487020892
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer 3 weeks ago,
but you ignore it silently. So I have copy my response here again, please you
give your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previous.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need. especially, if we enable some advance feature(e.g.
AddressSanitizer,
Stack Smashing Protector). Please read the related document to understand
what will happen behind the scene with the memroy layout if you aren't fimilar
with these technique.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memroy layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previous.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memroy layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memroy layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memory layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen behind the scene with the memory layout if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen behind the scene with the memory layout if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more temporary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen behind the scene with the memory layout if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more temporary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen with the memory layout behind the scene if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> Well the assembly code works. and it is not portable.
>
> I did not consider e.g. AddressSanitizer, Stack Smashing Protector. - So
you may be correct this may not work.
>
Then how do I handle this with your method? You need give a solution, right?
> **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
I can't understand what you mean. I don't change the fill value(all value
same as before) in this patch. If I am wrong, please point the code location I
will correct them.
>
> Here is how I was looking at it. 1) I Draw a picture of the stack frame
> [ ]
> [ ]
> [ ]
> [ ]<---At the function
> [ marker1]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker2 ]
> [ ] <- the allocation
>
> if the compiler messes with you and it looks like this, does not matter.
>
> [ marker2]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker1 ]
> [ ] <- the allocation
>
> safe = min of (&marker2, &marker1) word gets you the lowest point on the
stack you can write to,
>
Then do you think it's the right thing to memset "[ ] <- the allocation"?!
> DO NOT Use MEMSET! (it will not write the correct pattern, and you already
know the call it a problem) Use a for() with the appropriate sized type
>
First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
"it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statment is right.
> Set the start address and write from bottom up or top down from or to
`safe`.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> Well the assembly code works. and it is not portable.
>
> I did not consider e.g. AddressSanitizer, Stack Smashing Protector. - So
you may be correct this may not work.
>
Then how do I handle this with your method? You need give a solution, right?
> **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
I can't understand what you mean. I don't change the fill value(all value
same as before) in this patch. If I am wrong, please point the code location I
will correct them.
>
> Here is how I was looking at it. 1) I Draw a picture of the stack frame
> [ ]
> [ ]
> [ ]
> [ ]<---At the function
> [ marker1]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker2 ]
> [ ] <- the allocation
>
> if the compiler messes with you and it looks like this, does not matter.
>
> [ marker2]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker1 ]
> [ ] <- the allocation
>
> safe = min of (&marker2, &marker1) word gets you the lowest point on the
stack you can write to,
>
Then do you think it's the right thing to memset "[ ] <- the allocation"?!
The allocation region may contain:
1. The variables we defined between marker1 and marker2, but the compiler
free to move them to here
2. The compiler may add more temporary variables as needed and put to here
3. ASAN or SSP may add the special mark to here too
Please give a solution before I can use your method.
> DO NOT Use MEMSET! (it will not write the correct pattern, and you already
know the call it a problem) Use a for() with the appropriate sized type
>
First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
"it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statment is right.
> Set the start address and write from bottom up or top down from or to
`safe`.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my question again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my question again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my question again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my reply again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> I may not understand what you are saying, I may be busy. I am not
ignoring you or making a negative statement.
Let me give you an example:
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_checkstack.c#L97
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_createstack.c#L198
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/common/up_internal.h#L69
Please tell me what's wrong avr colorize the stack with memset and how your
approach handle this case? I aks the same question two months ago, this is a
key reason why I don't take your approach initially. Here is a capture:
Could you answer my question?
>
> I am letting you know you will break working code if make the change
across the all the ARCHs of the system. We have had that happen before.
The system is already in broken state now, all arch which forget to set idle
thread info can't boot into shell if they match:
1.Enable CONFIG_RTC
2.Enable CONFIG_LIBC_LOCALTIME
We found this issue on simulator two months ago, the problem is still there,
no any progress!
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> I may not understand what you are saying, I may be busy. I am not
ignoring you or making a negative statement.
Let me give you an example:
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_checkstack.c#L97
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_createstack.c#L198
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/common/up_internal.h#L69
Please tell me what's wrong avr colorize the stack with memset? I aks the
same question two months ago, this is a key reason why I don't take your
approach initially. Here is a capture:
Could you answer my question and a workable solution to handle this case?
>
> I am letting you know you will break working code if make the change
across the all the ARCHs of the system. We have had that happen before.
The system is already in broken state now, all arch which forget to set idle
thread info can't boot into shell if they match:
1.Enable CONFIG_RTC
2.Enable CONFIG_LIBC_LOCALTIME
We found this issue on simulator two months ago, the problem is still there,
no any progress!
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> I may not understand what you are saying, I may be busy. I am not
ignoring you or making a negative statement.
Let me give you an example:
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_checkstack.c#L97
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_createstack.c#L198
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/common/up_internal.h#L69
Please tell me what's wrong avr colorize the stack with memset? I aks the
same question two months ago, this is a key reason why I don't take your
approach initially. Here is a capture:
Could you answer my question and give a workable solution to handle this
case?
>
> I am letting you know you will break working code if make the change
across the all the ARCHs of the system. We have had that happen before.
The system is already in broken state now, all arch which forget to set idle
thread info can't boot into shell if they match:
1.Enable CONFIG_RTC
2.Enable CONFIG_LIBC_LOCALTIME
We found this issue on simulator two months ago, the problem is still there,
no any progress!
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer 3 weeks ago,
but you ignore it silently. So I have copy my response here again, please you
give your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previous.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need. especially, if we enable some advance feature(e.g.
AddressSanitizer,
Stack Smashing Protector). Please read the related document to understand
what will happen behind the scene with the memroy layout if you aren't fimilar
with these technique.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memroy layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previous.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have copy my response here again, please
you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memroy layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memroy layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker. Especially, if we
enable some advance feature(e.g. AddressSanitizer, Stack Smashing Protector).
Please read the related document to understand what will happen behind the
scene with the memory layout if you aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> @xiaoxiang781216 I am thinking along theses lines:
>
> Auto size it.
>
> If the passed pointer is on the current stack. I guess (pid == 0) would
work, but generic would be better;
>
> If you write from stack botom to current lowest marker on the stack.
>
> ```
> int arm_stack_color(stack, size)
> {
> volatile uint32_t marker1;
> register real vars...;
> uint32_t nos;
> volatile uint32_t marker2;
> if (check stack is is mine)
> {
> /* make no assumptions */
> nos = min( &marker2, &marker1);
> size = nos - stack;
> }
> ```
Two issues here:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset.
2.The code isn't portable at least in the theroy, because the compiler
permit to move "register real vars...;" out of the marker.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
if (tcb->pid == 0)
{
/* The whole idle thread stack can't be colored here
* because the code is running on the idle thead now.
*/
memset(tcb->stack_alloc_ptr, 0xaa, stack_size - STACK_MARGIN_IDLE);
}
else
{
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
}
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen behind the scene with the memory layout if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more tempary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen behind the scene with the memory layout if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more temporary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen behind the scene with the memory layout if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > ping again, @davids5 this patch is pending for more than two months,
could you answer my question a little bit timely?
>
> What I suggested will work, you understanding it may not. This will safely
get the the margin. Why don't you try it,
Yes, I want to try, but it can't work and I have give my answer two months
ago, but you ignore it silently. So I have to copy my response here again,
please you give me your comment one by one, so I can try with your new propose:
1.Many arch don't have xxx_stack_color, but call memset directly, then we
have to add some margin again for memset. Here is an example:
```
#ifdef CONFIG_STACK_COLORATION
memset(tcb->stack_alloc_ptr, 0xaa, stack_size);
#endif
```
Pleaase tell me how can I measure memset stack consumption with your method?
Do you want me to add the marker into memset?
2.The code isn't portable at least in the theory, because the compiler
permit to move "register real vars...;" out of the marker or add more temporary
variables as need. Especially, if we enable some advance feature(e.g.
AddressSanitizer, Stack Smashing Protector). Please read the related document
to understand what will happen with the memory layout behind the scene if you
aren't fimilar with these technique.
Actually before I made the change in this way, I have consider your propose
carefully and give up finally due the reason I mention previously.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> Well the assembly code works. and it is not portable.
>
> I did not consider e.g. AddressSanitizer, Stack Smashing Protector. - So
you may be correct this may not work.
>
Then how do I handle this with your method? You need give a solution, right?
> **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
I can't understand what you mean. I don't change the fill value(all value
same as before) in this patch. If I am wrong, please point the code location I
will correct them.
>
> Here is how I was looking at it. 1) I Draw a picture of the stack frame
> [ ]
> [ ]
> [ ]
> [ ]<---At the function
> [ marker1]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker2 ]
> [ ] <- the allocation
>
> if the compiler messes with you and it looks like this, does not matter.
>
> [ marker2]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker1 ]
> [ ] <- the allocation
>
> safe = min of (&marker2, &marker1) word gets you the lowest point on the
stack you can write to,
>
Then do you think it's the right thing to memset "[ ] <- the allocation"?!
> DO NOT Use MEMSET! (it will not write the correct pattern, and you already
know the call it a problem) Use a for() with the appropriate sized type
>
First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
"it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statment is right.
> Set the start address and write from bottom up or top down from or to
`safe`.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> Well the assembly code works. and it is not portable.
>
> I did not consider e.g. AddressSanitizer, Stack Smashing Protector. - So
you may be correct this may not work.
>
Then how do I handle this with your method? You need give a solution, right?
> **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
I can't understand what you mean. I don't change the fill value(all value
same as before) in this patch. If I am wrong, please point the code location I
will correct them.
>
> Here is how I was looking at it. 1) I Draw a picture of the stack frame
> [ ]
> [ ]
> [ ]
> [ ]<---At the function
> [ marker1]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker2 ]
> [ ] <- the allocation
>
> if the compiler messes with you and it looks like this, does not matter.
>
> [ marker2]
> [ ]
> [ what is here or not does not matter ]
> [ ]
> [ marker1 ]
> [ ] <- the allocation
>
> safe = min of (&marker2, &marker1) word gets you the lowest point on the
stack you can write to,
>
Then do you think it's the right thing to memset "[ ] <- the allocation"?!
The allocation region may contain:
1. The variables we defined between marker1 and marker2, but the compiler
free to move them to here
2. The compiler may add more temporary variables as needed and put to here
3. ASAN or SSP may add the special mark to here too
Please give a solution before I can use your method.
> DO NOT Use MEMSET! (it will not write the correct pattern, and you already
know the call it a problem) Use a for() with the appropriate sized type
>
First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
"it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statment is right.
> Set the start address and write from bottom up or top down from or to
`safe`.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my question again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my question again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my question again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> > > Well the assembly code works. and it is not portable.
> > > I did not consider e.g. AddressSanitizer, Stack Smashing Protector. -
So you may be correct this may not work.
> >
> >
> > Then how do I handle this with your method? You need give a solution,
right?
> > > **Please Note: Fill the stack ONLY with the fill value as defined from
before or you will break all the monitors.**
> >
> >
> > I can't understand what you mean. I don't change the fill value(all
value same as before) in this patch. If I am wrong, please point the code
location I will correct them.
> > > Here is how I was looking at it. 1) I Draw a picture of the stack frame
> > > [ ]
> > > [ ]
> > > [ ]
> > > [ ]<---At the function
> > > [ marker1]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker2 ]
> > > [ ] <- the allocation
> > > if the compiler messes with you and it looks like this, does not
matter.
> > > [ marker2]
> > > [ ]
> > > [ what is here or not does not matter ]
> > > [ ]
> > > [ marker1 ]
> > > [ ] <- the allocation
> > > safe = min of (&marker2, &marker1) word gets you the lowest point on
the stack you can write to,
> >
> >
> > Then do you think it's the right thing to memset "[ ] <- the
allocation"?!
> > The allocation region may contain:
> >
> > 1. The variables we defined between marker1 and marker2, but the
compiler free to move them to here
> > 2. The compiler may add more temporary variables as needed and put to
here
> > 3. ASAN or SSP may add the special mark to here too
> >
> > Please give a solution before I can use your method.
>
> I would make it a simple function and make it work. I would test it on
different archs,
>
As I said before, it work with the current compiler and option, doesn't mean
it work in the furture. So your propose isn't bettter than what PR done.
> If you are not sure the the compiler will do. Write it in assembly .
>
Yes, that's why I ask you two months ago, let me quote it here to remember
you:
> If you like go_nx_start approach, I can keep go_nx_start as before and let
up_use_stack skip to colorize the idle thread stack. But, I don't have time to
implement go_nx_start for each SoC.
But you ignore my reply again.
> > > DO NOT Use MEMSET! (it will not write the correct pattern, and you
already know the call it a problem) Use a for() with the appropriate sized type
> >
> >
> > First, the original code use memset, not my patch. Second, why not use
memset? the orignial code work well.
>
> This is the original code
>
https://github.com/PX4/NuttX/blob/ec20f2e6c5cc35b2b9bbe942dea55eabb81297b6/arch/arm/src/stm32/stm32_start.c#L225-L256
>
> It does the correct thing.
>
> > "it will not write the correct pattern", could you explain the reason?
Sorry, I don't think this statement is right.
>
> The pattern is STACK_COLOR = 0xdeadbeef
>
> This is what is used to check penetration and it MUST match. Or you will
break the detection,.
I kindly point out memset serveral time, do you look at the realated code
before you make any negative statement? On the other arch, STACK_COLOR equals
0xaa!!! So please review my patch fully, please.
>
> > > Set the start address and write from bottom up or top down from or to
`safe`.
>
> Set the start address and write from bottom up to safe
> or
> write from top down from `safe` to the base.
>
> I feel we saying the same thing over and over again and not making
progress. Have you looked at the code generated and determined this will not
work or is that your opinion?
>
> If you post the generated code that proves you can not do it across the
permutations of ARCH, ASAN or SSP that will prove it does not work.
>
> You have to realize that there is also a risk with the #define approach,
that cause a problem when the call level get pushed deeper.
Yes, both approach aren't perfect, that's why I don't accept your propose.
Actually, I initially take your propose but find that many arch call memset to
colorize the stack which make your propose is impossible to complete.
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> I may not understand what you are saying, I may be busy. I am not
ignoring you or making a negative statement.
Let me give you an example:
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_checkstack.c#L97
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_createstack.c#L198
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/common/up_internal.h#L69
Please tell me what's wrong avr colorize the stack with memset and how your
approach handle this case? I aks the same question two months ago, this is a
key reason why I don't take your approach initially. Here is a capture:
Could you answer my question?
>
> I am letting you know you will break working code if make the change
across the all the ARCHs of the system. We have had that happen before.
The system is already in broken state now, all arch which forget to set idle
thread info can't boot into shell if they match:
1.Enable CONFIG_RTC
2.Enable CONFIG_LIBC_LOCALTIME
We found this issue on simulator two months ago, the problem is still there,
no any progress!
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> I may not understand what you are saying, I may be busy. I am not
ignoring you or making a negative statement.
Let me give you an example:
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_checkstack.c#L97
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_createstack.c#L198
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/common/up_internal.h#L69
Please tell me what's wrong avr colorize the stack with memset? I aks the
same question two months ago, this is a key reason why I don't take your
approach initially. Here is a capture:
Could you answer my question and a workable solution to handle this case?
>
> I am letting you know you will break working code if make the change
across the all the ARCHs of the system. We have had that happen before.
The system is already in broken state now, all arch which forget to set idle
thread info can't boot into shell if they match:
1.Enable CONFIG_RTC
2.Enable CONFIG_LIBC_LOCALTIME
We found this issue on simulator two months ago, the problem is still there,
no any progress!
##########
File path: arch/arm/src/arm/arm_initialstate.c
##########
@@ -52,13 +52,7 @@
* Pre-processor Definitions
****************************************************************************/
-/****************************************************************************
- * Private Data
- ****************************************************************************/
-
-/****************************************************************************
- * Private Functions
- ****************************************************************************/
+#define IDLETHREAD_STACKMARGIN 128
Review comment:
> I may not understand what you are saying, I may be busy. I am not
ignoring you or making a negative statement.
Let me give you an example:
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_checkstack.c#L97
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/avr/up_createstack.c#L198
https://github.com/apache/incubator-nuttx/blob/master/arch/avr/src/common/up_internal.h#L69
Please tell me what's wrong avr colorize the stack with memset? I aks the
same question two months ago, this is a key reason why I don't take your
approach initially. Here is a capture:
Could you answer my question and give a workable solution to handle this
case?
>
> I am letting you know you will break working code if make the change
across the all the ARCHs of the system. We have had that happen before.
The system is already in broken state now, all arch which forget to set idle
thread info can't boot into shell if they match:
1.Enable CONFIG_RTC
2.Enable CONFIG_LIBC_LOCALTIME
We found this issue on simulator two months ago, the problem is still there,
no any progress!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
