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/03/26 08:05:06 UTC
[GitHub] [incubator-nuttx] yamt opened a new pull request #629: sim: 64-bit
modules on macOS
yamt opened a new pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398422586
##########
File path: arch/sim/src/sim/up_modtext.c
##########
@@ -0,0 +1,104 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_modtext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* Note: this module assumes that size_t is compatible between
+ * nuttx and host.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/mman.h>
+
+#include <assert.h>
+#include <stddef.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct module_text_header
+{
+ size_t size;
+};
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_module_text_init
+ *
+ * Description:
+ * Initialize the module text allocator
+ *
+ ****************************************************************************/
+
+void up_module_text_init()
+{
+}
+
+/****************************************************************************
+ * Name: up_module_text_alloc
+ *
+ * Description:
+ * Allocate memory for module text.
+ *
+ ****************************************************************************/
+
+void *up_module_text_alloc(size_t size)
+{
+ struct module_text_header *hdr;
+
+ hdr = mmap(NULL, sizeof(*hdr) + size, PROT_READ | PROT_WRITE | PROT_EXEC,
Review comment:
Why we can't directly use the main heap?
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398428098
##########
File path: arch/sim/src/sim/up_modtext.c
##########
@@ -0,0 +1,104 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_modtext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* Note: this module assumes that size_t is compatible between
+ * nuttx and host.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/mman.h>
+
+#include <assert.h>
+#include <stddef.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct module_text_header
+{
+ size_t size;
+};
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_module_text_init
+ *
+ * Description:
+ * Initialize the module text allocator
+ *
+ ****************************************************************************/
+
+void up_module_text_init()
+{
+}
+
+/****************************************************************************
+ * Name: up_module_text_alloc
+ *
+ * Description:
+ * Allocate memory for module text.
+ *
+ ****************************************************************************/
+
+void *up_module_text_alloc(size_t size)
+{
+ struct module_text_header *hdr;
+
+ hdr = mmap(NULL, sizeof(*hdr) + size, PROT_READ | PROT_WRITE | PROT_EXEC,
Review comment:
> from the commit message:
>
> > On modern environments, sim_heap on bss is not executable.
> > Enable only on macOS for now because I haven't investigated others.
we can change sim_heap to mmap/mumap directly, this change is more simpler.
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399006290
##########
File path: boards/sim/sim/sim/configs/cxxtest/Make.defs
##########
@@ -86,6 +86,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
Review comment:
> Mach-O
Can we use --oformat to tell ld generate elf format?
```
--oformat output-format
ld may be configured to support more than one kind of object file. If your
ld is configured this way, you can use the ‘--oformat’ option to specify the
binary format for the output object file. Even when ld is configured to support
alternative object formats, you don’t usually need to specify this, as ld should
be configured to produce as a default output format the most usual format on
each machine. output-format is a text string, the name of a particular format
supported by the BFD libraries. (You can list the available binary formats
with ‘objdump -i’.) The script command OUTPUT_FORMAT can also specify the
output format, but this option overrides it. See Chapter 5 [BFD], page 69.
```
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398425821
##########
File path: boards/sim/sim/sim/configs/cxxtest/Make.defs
##########
@@ -86,6 +86,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
Review comment:
because the native binary format is Mach-O.
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399015246
##########
File path: arch/sim/src/sim/up_allocateheap.c
##########
@@ -71,6 +75,15 @@ static uint8_t sim_heap[SIM_HEAP_SIZE];
void up_allocate_heap(void **heap_start, size_t *heap_size)
{
+ /* Note: Some subsystems like modlib and binfmt need to allocate
+ * executable memory. We make the entire heap executable here to keep
+ * the sim simpler. If it turns out to be a problem, the
+ * ARCH_HAVE_MODULE_TEXT mechanism can be an alternative.
+ */
+
+#if defined(CONFIG_LIBC_MODLIB) || defined(CONFIG_BINFMT_LOADABLE)
Review comment:
How about we change to:
```
#if defined(CONFIG_LIBC_MODLIB) || defined(CONFIG_BINFMT_LOADABLE)
*heap_start = host_alloc_heap(SIM_HEAP_SIZE);
#else
static uint8_t sim_heap[SIM_HEAP_SIZE];
*heap_start = sim_heap;
#endif
```
And remove the line 55-59.
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398455002
##########
File path: arch/sim/src/sim/up_allocateheap.c
##########
@@ -71,6 +75,9 @@ static uint8_t sim_heap[SIM_HEAP_SIZE];
void up_allocate_heap(void **heap_start, size_t *heap_size)
{
+#if defined(CONFIG_LIBC_MODLIB)
Review comment:
elf binaries are out of the scope of this PR. :-)
i don't know if mmap works on windows. i guess not.
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399006290
##########
File path: boards/sim/sim/sim/configs/cxxtest/Make.defs
##########
@@ -86,6 +86,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
Review comment:
> because the native binary format is Mach-O.
Can we use --oformat to tell ld generate elf format?
```
--oformat output-format
ld may be configured to support more than one kind of object file. If your
ld is configured this way, you can use the ‘--oformat’ option to specify the
binary format for the output object file. Even when ld is configured to support
alternative object formats, you don’t usually need to specify this, as ld should
be configured to produce as a default output format the most usual format on
each machine. output-format is a text string, the name of a particular format
supported by the BFD libraries. (You can list the available binary formats
with ‘objdump -i’.) The script command OUTPUT_FORMAT can also specify the
output format, but this option overrides it. See Chapter 5 [BFD], page 69.
```
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398510978
##########
File path: arch/sim/src/sim/up_allocateheap.c
##########
@@ -71,6 +75,9 @@ static uint8_t sim_heap[SIM_HEAP_SIZE];
void up_allocate_heap(void **heap_start, size_t *heap_size)
{
+#if defined(CONFIG_LIBC_MODLIB)
Review comment:
> elf binaries are out of the scope of this PR. :-)
>
But, elf binary should has the same problem, why we don't fix together?
> i don't know if mmap works on windows. i guess not.
How about we check BINFMT 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
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399023140
##########
File path: arch/sim/src/sim/up_allocateheap.c
##########
@@ -71,6 +75,15 @@ static uint8_t sim_heap[SIM_HEAP_SIZE];
void up_allocate_heap(void **heap_start, size_t *heap_size)
{
+ /* Note: Some subsystems like modlib and binfmt need to allocate
+ * executable memory. We make the entire heap executable here to keep
+ * the sim simpler. If it turns out to be a problem, the
+ * ARCH_HAVE_MODULE_TEXT mechanism can be an alternative.
+ */
+
+#if defined(CONFIG_LIBC_MODLIB) || defined(CONFIG_BINFMT_LOADABLE)
Review comment:
done
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398426160
##########
File path: arch/sim/src/sim/up_modtext.c
##########
@@ -0,0 +1,104 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_modtext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* Note: this module assumes that size_t is compatible between
+ * nuttx and host.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/mman.h>
+
+#include <assert.h>
+#include <stddef.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct module_text_header
+{
+ size_t size;
+};
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_module_text_init
+ *
+ * Description:
+ * Initialize the module text allocator
+ *
+ ****************************************************************************/
+
+void up_module_text_init()
+{
+}
+
+/****************************************************************************
+ * Name: up_module_text_alloc
+ *
+ * Description:
+ * Allocate memory for module text.
+ *
+ ****************************************************************************/
+
+void *up_module_text_alloc(size_t size)
+{
+ struct module_text_header *hdr;
+
+ hdr = mmap(NULL, sizeof(*hdr) + size, PROT_READ | PROT_WRITE | PROT_EXEC,
Review comment:
from the commit message:
> On modern environments, sim_heap on bss is not executable.
> Enable only on macOS for now because I haven't investigated others.
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399018274
##########
File path: boards/sim/sim/sim/configs/nsh2/Make.defs
##########
@@ -82,6 +82,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
+ MODULECC = x86_64-elf-gcc
Review comment:
it doesn't work as here we are overriding the default value from Config.mk.
a user can override it by "make MODULECC=foo" even with the current code.
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398429347
##########
File path: arch/sim/src/sim/up_modtext.c
##########
@@ -0,0 +1,104 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_modtext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* Note: this module assumes that size_t is compatible between
+ * nuttx and host.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/mman.h>
+
+#include <assert.h>
+#include <stddef.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct module_text_header
+{
+ size_t size;
+};
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_module_text_init
+ *
+ * Description:
+ * Initialize the module text allocator
+ *
+ ****************************************************************************/
+
+void up_module_text_init()
+{
+}
+
+/****************************************************************************
+ * Name: up_module_text_alloc
+ *
+ * Description:
+ * Allocate memory for module text.
+ *
+ ****************************************************************************/
+
+void *up_module_text_alloc(size_t size)
+{
+ struct module_text_header *hdr;
+
+ hdr = mmap(NULL, sizeof(*hdr) + size, PROT_READ | PROT_WRITE | PROT_EXEC,
Review comment:
maybe. but in general it's a good thing to have the heap non executable.
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399009706
##########
File path: tools/Config.mk
##########
@@ -363,3 +363,9 @@ define TESTANDREPLACEFILE
fi
endef
endif
+
+# Some defaults.
+# $(TOPDIR)/Make.defs can override these appropriately.
+
+MODULECC = $(CC)
Review comment:
i'd suggest
* MODULECC for modules
* ELFCC for ELF binaries
to align with the existing CMODULEFLAGS/CELFFLAGS
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399018326
##########
File path: arch/sim/src/sim/up_allocateheap.c
##########
@@ -71,6 +75,15 @@ static uint8_t sim_heap[SIM_HEAP_SIZE];
void up_allocate_heap(void **heap_start, size_t *heap_size)
{
+ /* Note: Some subsystems like modlib and binfmt need to allocate
+ * executable memory. We make the entire heap executable here to keep
+ * the sim simpler. If it turns out to be a problem, the
+ * ARCH_HAVE_MODULE_TEXT mechanism can be an alternative.
+ */
+
+#if defined(CONFIG_LIBC_MODLIB) || defined(CONFIG_BINFMT_LOADABLE)
Review comment:
will do
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399006290
##########
File path: boards/sim/sim/sim/configs/cxxtest/Make.defs
##########
@@ -86,6 +86,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
Review comment:
> Mach-O
Can we use --oformat to tell ld generate elf format?
```
--oformat output-format
ld may be configured to support more than one kind of object file. If your
ld is configured this way, you can use the ‘--oformat’ option to specify the
binary format for the output object file. Even when ld is configured to support
alternative object formats, you don’t usually need to specify this, as ld should
be configured to produce as a default output format the most usual format on
each machine. output-format is a text string, the name of a particular format
supported by the BFD libraries. (You can list the available binary formats
with ‘objdump -i’.) The script command OUTPUT_FORMAT can also specify the
output format, but this option overrides it. See Chapter 5 [BFD], page 69.
```
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398448676
##########
File path: arch/sim/src/sim/up_modtext.c
##########
@@ -0,0 +1,104 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_modtext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* Note: this module assumes that size_t is compatible between
+ * nuttx and host.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/mman.h>
+
+#include <assert.h>
+#include <stddef.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct module_text_header
+{
+ size_t size;
+};
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_module_text_init
+ *
+ * Description:
+ * Initialize the module text allocator
+ *
+ ****************************************************************************/
+
+void up_module_text_init()
+{
+}
+
+/****************************************************************************
+ * Name: up_module_text_alloc
+ *
+ * Description:
+ * Allocate memory for module text.
+ *
+ ****************************************************************************/
+
+void *up_module_text_alloc(size_t size)
+{
+ struct module_text_header *hdr;
+
+ hdr = mmap(NULL, sizeof(*hdr) + size, PROT_READ | PROT_WRITE | PROT_EXEC,
Review comment:
done.
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398453177
##########
File path: arch/sim/src/sim/up_allocateheap.c
##########
@@ -71,6 +75,9 @@ static uint8_t sim_heap[SIM_HEAP_SIZE];
void up_allocate_heap(void **heap_start, size_t *heap_size)
{
+#if defined(CONFIG_LIBC_MODLIB)
Review comment:
How about elf binary? or let's always host_alloc_heap.
----------------------------------------------------------------
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 merged pull request #629: sim:
64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399014652
##########
File path: tools/Config.mk
##########
@@ -363,3 +363,9 @@ define TESTANDREPLACEFILE
fi
endef
endif
+
+# Some defaults.
+# $(TOPDIR)/Make.defs can override these appropriately.
+
+MODULECC = $(CC)
Review comment:
Ok. Change to:
MODULECC ?= $(CC)?
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398440829
##########
File path: arch/sim/src/sim/up_modtext.c
##########
@@ -0,0 +1,104 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_modtext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* Note: this module assumes that size_t is compatible between
+ * nuttx and host.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/mman.h>
+
+#include <assert.h>
+#include <stddef.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct module_text_header
+{
+ size_t size;
+};
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_module_text_init
+ *
+ * Description:
+ * Initialize the module text allocator
+ *
+ ****************************************************************************/
+
+void up_module_text_init()
+{
+}
+
+/****************************************************************************
+ * Name: up_module_text_alloc
+ *
+ * Description:
+ * Allocate memory for module text.
+ *
+ ****************************************************************************/
+
+void *up_module_text_alloc(size_t size)
+{
+ struct module_text_header *hdr;
+
+ hdr = mmap(NULL, sizeof(*hdr) + size, PROT_READ | PROT_WRITE | PROT_EXEC,
Review comment:
ok, it makes sense.
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399006952
##########
File path: tools/Config.mk
##########
@@ -363,3 +363,9 @@ define TESTANDREPLACEFILE
fi
endef
endif
+
+# Some defaults.
+# $(TOPDIR)/Make.defs can override these appropriately.
+
+MODULECC = $(CC)
Review comment:
Should we select a better name? because elf binary rule also need to use macro.
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399014448
##########
File path: boards/sim/sim/sim/configs/nsh2/Make.defs
##########
@@ -82,6 +82,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
+ MODULECC = x86_64-elf-gcc
Review comment:
Should we change to:
MODULECC ?= x86_64-elf-gcc
so the user have a chance to overwrite the toolchain?
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398435789
##########
File path: arch/sim/src/sim/up_modtext.c
##########
@@ -0,0 +1,104 @@
+/****************************************************************************
+ * arch/sim/src/sim/up_modtext.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/* Note: this module assumes that size_t is compatible between
+ * nuttx and host.
+ */
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <sys/mman.h>
+
+#include <assert.h>
+#include <stddef.h>
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct module_text_header
+{
+ size_t size;
+};
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: up_module_text_init
+ *
+ * Description:
+ * Initialize the module text allocator
+ *
+ ****************************************************************************/
+
+void up_module_text_init()
+{
+}
+
+/****************************************************************************
+ * Name: up_module_text_alloc
+ *
+ * Description:
+ * Allocate memory for module text.
+ *
+ ****************************************************************************/
+
+void *up_module_text_alloc(size_t size)
+{
+ struct module_text_header *hdr;
+
+ hdr = mmap(NULL, sizeof(*hdr) + size, PROT_READ | PROT_WRITE | PROT_EXEC,
Review comment:
> maybe. but in general it's a good thing to have the heap non executable.
But, the new change make the code much complex. The most usage for sim is for developing and the security impact of executable heap is small.
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398523093
##########
File path: arch/sim/src/sim/up_allocateheap.c
##########
@@ -71,6 +75,9 @@ static uint8_t sim_heap[SIM_HEAP_SIZE];
void up_allocate_heap(void **heap_start, size_t *heap_size)
{
+#if defined(CONFIG_LIBC_MODLIB)
Review comment:
ok. done.
----------------------------------------------------------------
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] yamt commented on a change in pull request #629:
sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r399008164
##########
File path: boards/sim/sim/sim/configs/cxxtest/Make.defs
##########
@@ -86,6 +86,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
Review comment:
macos' ld doesn't a have -oformat option.
i don't know if it's possible to use gnu ld for mach-o.
it's simpler to use a separate toolchain for different targets.
----------------------------------------------------------------
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 #629: sim: 64-bit modules on macOS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #629: sim: 64-bit modules on macOS
URL: https://github.com/apache/incubator-nuttx/pull/629#discussion_r398420328
##########
File path: boards/sim/sim/sim/configs/cxxtest/Make.defs
##########
@@ -86,6 +86,18 @@ else
LDMODULEFLAGS += -T $(TOPDIR)/libs/libc/modlib/gnu-elf.ld
endif
+# NuttX modules are ELF binaries.
+# Non-ELF platforms like macOS need to use a separate ELF toolchain.
+ifeq ($(CONFIG_HOST_MACOS),y)
+ # eg. brew install x86_64-elf-gcc
Review comment:
Why we don't use x86_64-elf-gcc for OS self 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