You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/14 09:00:17 UTC
[GitHub] [incubator-nuttx] yamt opened a new pull request #3546: modlib: Implement sh_addralign handling
yamt opened a new pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546
## Summary
I've seen a module with 16 bytes .rodata alignment for xmm operations.
It was getting SEGV on sim/Linux because of the alignment issue.
## Impact
CONFIG_MODULE
## Testing
The same module binary seems working fine after applying this patch.
Also, tested on sim/macOS and esp32 on qemu,
using a module with an artificially large alignment. (64 bytes)
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613112170
##########
File path: include/nuttx/lib/modlib.h
##########
@@ -198,6 +198,8 @@ struct mod_loadinfo_s
uintptr_t datastart; /* Start of.bss/.data memory in .text allocation */
size_t textsize; /* Size of the module .text memory allocation */
size_t datasize; /* Size of the module .bss/.data memory allocation */
+ unsigned int textalign; /* Necessary alignment of .text */
+ unsigned int dataalign; /* Necessary alignment of .bss/.text */
Review comment:
should we use size_t like textsize/datasize?
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613169452
##########
File path: include/nuttx/lib/modlib.h
##########
@@ -198,6 +198,8 @@ struct mod_loadinfo_s
uintptr_t datastart; /* Start of.bss/.data memory in .text allocation */
size_t textsize; /* Size of the module .text memory allocation */
size_t datasize; /* Size of the module .bss/.data memory allocation */
+ unsigned int textalign; /* Necessary alignment of .text */
+ unsigned int dataalign; /* Necessary alignment of .bss/.text */
Review comment:
maybe. i don't think that large alignment is realistic though.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613168335
##########
File path: libs/libc/modlib/modlib_load.c
##########
@@ -271,16 +293,29 @@ int modlib_load(FAR struct mod_loadinfo_s *loadinfo)
}
}
#else
- loadinfo->textalloc = (uintptr_t)lib_malloc(loadinfo->textsize +
- loadinfo->datasize);
+ align = loadinfo->textalign;
+ if (align < loadinfo->dataalign)
+ {
+ align = loadinfo->dataalign;
+ }
+
+ text_size = loadinfo->textsize;
+ if (loadinfo->datasize > 0)
+ {
+ text_size = _ALIGN_UP(text_size, loadinfo->dataalign);
+ }
+
+ loadinfo->textalloc = (uintptr_t)lib_memalign(align,
Review comment:
i dunno. i guess it has both of pros and cons.
iirc, the original code was doing a single allocation and i changed it for CONFIG_ARCH_USE_MODULE_TEXT case.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613649367
##########
File path: libs/libc/modlib/modlib_load.c
##########
@@ -271,16 +293,29 @@ int modlib_load(FAR struct mod_loadinfo_s *loadinfo)
}
}
#else
- loadinfo->textalloc = (uintptr_t)lib_malloc(loadinfo->textsize +
- loadinfo->datasize);
+ align = loadinfo->textalign;
+ if (align < loadinfo->dataalign)
+ {
+ align = loadinfo->dataalign;
+ }
+
+ text_size = loadinfo->textsize;
+ if (loadinfo->datasize > 0)
+ {
+ text_size = _ALIGN_UP(text_size, loadinfo->dataalign);
+ }
+
+ loadinfo->textalloc = (uintptr_t)lib_memalign(align,
Review comment:
i'm ok with making it always use separate allocations. do you want me to do it in this PR? i'm feeling that a separate PR is more appropriate.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613710639
##########
File path: libs/libc/modlib/modlib_load.c
##########
@@ -271,16 +293,29 @@ int modlib_load(FAR struct mod_loadinfo_s *loadinfo)
}
}
#else
- loadinfo->textalloc = (uintptr_t)lib_malloc(loadinfo->textsize +
- loadinfo->datasize);
+ align = loadinfo->textalign;
+ if (align < loadinfo->dataalign)
+ {
+ align = loadinfo->dataalign;
+ }
+
+ text_size = loadinfo->textsize;
+ if (loadinfo->datasize > 0)
+ {
+ text_size = _ALIGN_UP(text_size, loadinfo->dataalign);
+ }
+
+ loadinfo->textalloc = (uintptr_t)lib_memalign(align,
Review comment:
Ok, it's fine to put into a new PR.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613166785
##########
File path: libs/libc/modlib/modlib_load.c
##########
@@ -271,16 +293,29 @@ int modlib_load(FAR struct mod_loadinfo_s *loadinfo)
}
}
#else
- loadinfo->textalloc = (uintptr_t)lib_malloc(loadinfo->textsize +
Review comment:
just because the existing code is different.
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613648726
##########
File path: include/nuttx/lib/modlib.h
##########
@@ -198,6 +198,8 @@ struct mod_loadinfo_s
uintptr_t datastart; /* Start of.bss/.data memory in .text allocation */
size_t textsize; /* Size of the module .text memory allocation */
size_t datasize; /* Size of the module .bss/.data memory allocation */
+ unsigned int textalign; /* Necessary alignment of .text */
+ unsigned int dataalign; /* Necessary alignment of .bss/.text */
Review comment:
fixed
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546
--
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3546: modlib: Implement sh_addralign handling
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3546:
URL: https://github.com/apache/incubator-nuttx/pull/3546#discussion_r613105669
##########
File path: libs/libc/modlib/modlib_load.c
##########
@@ -271,16 +293,29 @@ int modlib_load(FAR struct mod_loadinfo_s *loadinfo)
}
}
#else
- loadinfo->textalloc = (uintptr_t)lib_malloc(loadinfo->textsize +
Review comment:
Why go through the different patch with or without CONFIG_ARCH_USE_MODULE_TEXT?
##########
File path: libs/libc/modlib/modlib_load.c
##########
@@ -271,16 +293,29 @@ int modlib_load(FAR struct mod_loadinfo_s *loadinfo)
}
}
#else
- loadinfo->textalloc = (uintptr_t)lib_malloc(loadinfo->textsize +
- loadinfo->datasize);
+ align = loadinfo->textalign;
+ if (align < loadinfo->dataalign)
+ {
+ align = loadinfo->dataalign;
+ }
+
+ text_size = loadinfo->textsize;
+ if (loadinfo->datasize > 0)
+ {
+ text_size = _ALIGN_UP(text_size, loadinfo->dataalign);
+ }
+
+ loadinfo->textalloc = (uintptr_t)lib_memalign(align,
Review comment:
Is it good to combine text and data in one allocation? Since it require more large consecutive memory.
--
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:
users@infra.apache.org