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 2020/02/07 07:11:56 UTC
[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #220:
ELF64 support
masayuki2009 opened a new pull request #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220
### Summary
- This PR adds ELF64 support to NuttX.
- To use ELF64, CONFIG_ELF_64BIT must be enabled.
- To support ELF64, some common definitions have been moved from elf32.h to elf_common.h
- Also, Elf_xxx have been introduced to be used in common libraries such as binfmt.
- However, Elf32_xxx are still valid to be mainly used in arch_elf.c (32bit CPU only)
- Add RISC-V 64bit ELF support
### Impact
- This PR affects common libraries such as binfmt but should work
### Limitations / TODO
- Currently 32bit ELF for RISC-V is not supported
### Testing
- Tested with following environments
- (32bit cpu) spresense:elf, spresense:posix_spawn, spresense:module
- (64bit cpu) maix-bit:elf, maix-bit:posix_spawn
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#discussion_r376333574
##########
File path: include/elf_common.h
##########
@@ -0,0 +1,214 @@
+/****************************************************************************
+ * include/elf_common.h
Review comment:
It is better to rename elf_common.h to elf.h to more compatible with other POSIX variant, and include elf32.h/elf64.h inside elf.h like this:
```
#ifndef __INCLUDE_ELF_H
#define __INCLUDE_ELF_H
...common definition
#ifdef CONFIG_ELF_64BIT
# include <elf64.h>
#else
# include <elf32.h>
#endif
#endif /* __INCLUDE_ELF_H */
```
And let other code include elf.h instead.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo merged pull request #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull
request #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#discussion_r376357543
##########
File path: binfmt/libelf/libelf_bind.c
##########
@@ -326,8 +367,181 @@ static int elf_relocate(FAR struct elf_loadinfo_s *loadinfo, int relidx,
static int elf_relocateadd(FAR struct elf_loadinfo_s *loadinfo, int relidx,
Review comment:
> Let's enhance modlib_relocateadd too,
> maybe it's better to split RELA to another patch.
@xiaoxiang781216 Thanks for the comments. I'll consider them later.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583502128
We all agreed to support these Inviolable principles when we started this project together.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #220: ELF64
support
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583384773
@patacongo Thanks for the comment regarding coding standard. Now I'm preparing new patch sets which @xiaoxiang781216 pointed out. Perhaps, I need one more day...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583417902
Yes, but it is not a standard.
I don't know what the answer is. I understand what you are saying, but if we ignore the coding standard, then I think we are also asking for problems in the future. I think it is safer to just always following the coding standard unless there is an established standard that conflicts with it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583501897
I don't think the "making things easier" is a argument for not following the coding style. That is spelled out in the Inviolables:
53 o Personal or organizational preference is not a justification for a
54 coding style change.
55 o Nothing can come into NuttX that does not follow the coding standard.
56 o Expediency is not a justification for violating the coding standard.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583380800
"Please note that some typedefs such as Elf32_xxx, Elf64_xxx, Elf_xxx still violate NuttX conding standard." Those are the names used in other elf header files. I have missed feelings if they should be made to match the coding standard or not. They are controlled by any standard, so the answer is that they should be made to follow the NuttX coding standard someday.
Given that Masayuki has suggested waiting for subsequent PRs to address the comments, is this PR ready to be merged now?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #220: ELF64
support
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583380800
"Please note that some typedefs such as Elf32_xxx, Elf64_xxx, Elf_xxx still violate NuttX conding standard." Those are the names used in other elf header files. I have mixed feelings if they should be made to match the coding standard or not. They are NOT controlled by any standard, so the answer is that they should be made to follow the NuttX coding standard someday.
Given that Masayuki has suggested waiting for subsequent PRs to address the comments, is this PR ready to be merged now?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583500778
I think we should follow the coding standard, but I have no problem with incorporating the PR as it is. Feel free to merge it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #220:
ELF64 support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583416696
> someday
But this is a defacto standard used in all POSIX variant, should we follow the well established convention?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #220: ELF64
support
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583265014
Please note that some typedefs such as Elf32_xxx, Elf64_xxx, Elf_xxx still violate NuttX conding standard. Because modifying all such typedefs impacts many files, so in this PR, I focused on Elf64 support with minimal changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#discussion_r376336057
##########
File path: binfmt/libelf/libelf_bind.c
##########
@@ -326,8 +367,181 @@ static int elf_relocate(FAR struct elf_loadinfo_s *loadinfo, int relidx,
static int elf_relocateadd(FAR struct elf_loadinfo_s *loadinfo, int relidx,
Review comment:
Let's enhance modlib_relocateadd too, maybe it's better to split RELA to another patch.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #220: ELF64
support
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583653259
I've modified the code and pushed with -f.
And tested with the following environments
(32bit cpu) spresense:elf, spresense:posix_spawn, spresense:module
(64bit cpu) maix-bit:elf, maix-bit:posix_spawn, maix-bit:module (newly added)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #220: ELF64
support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583416696
> someday
But this is a defacto standard, should we follow the well established convention?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull
request #220: ELF64 support
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#discussion_r376355930
##########
File path: include/elf_common.h
##########
@@ -0,0 +1,214 @@
+/****************************************************************************
+ * include/elf_common.h
Review comment:
> It is better to rename elf_common.h to elf.h to more compatible with other POSIX variant,
> and include elf32.h/elf64.h inside elf.h like this:
@xiaoxiang781216 Thanks for the comment. I'll modify the file later.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #220: ELF64
support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #220: ELF64 support
URL: https://github.com/apache/incubator-nuttx/pull/220#issuecomment-583441002
> Yes, but it is not a standard.
>
> I don't know what the answer is. I understand what you are saying, but if we ignore the coding standard, then I think we are also asking for problems in the future. I think it is safer to just always following the coding standard unless there is an established standard that conflicts with it.
Why we follow the standard? I think the most important reason is the compatiblity, documentation and knowledge share, is it right? so if we follow this principle, it's better to continue the convention if some functions/structure are already accepted by multiple mature OS for many years even they haven't been standardized yet, because:
1.These are most likely the base for the next standard revision and then become the standard.
2.Improve the compatibility since many software already depends on this well established convention.
3.It's also consistent with some function/structure we already borrowed from Linux.
So I think the standard should be extended to cover the defacto standard(well established convention) too.
----------------------------------------------------------------
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
With regards,
Apache Git Services