xiaoxiang781216 commented on code in PR #20492:
URL: https://github.com/apache/nuttx/pull/20492#discussion_r4303412709
##########
binfmt/binfmt_loadmodule.c:
##########
@@ -184,6 +184,12 @@ int load_module(FAR struct binary_s *bin, FAR const char
*filename,
if (bin && filename)
#endif
{
+#ifdef CONFIG_LIBC_ELF
+ /* The DT_NEEDED libraries of the program go here */
+
+ list_initialize(&bin->libraries);
Review Comment:
libraries may need for other format too, remove the check CONFIG_LIBC_ELF
##########
binfmt/binfmt_execmodule.c:
##########
@@ -45,6 +45,7 @@
#include <nuttx/spawn.h>
#include <nuttx/binfmt/binfmt.h>
#include <nuttx/environ.h>
+#include <nuttx/lib/elf.h>
Review Comment:
shouldn't include elf.h in the common loader code
##########
libs/libc/elf/elf_getsymbol.c:
##########
@@ -73,13 +78,12 @@ FAR const void *libelf_getsymbol(FAR void *handle, FAR
const char *name)
/* Verify that the module is in the registry */
- libelf_registry_lock();
- ret = libelf_registry_verify(modp);
+ ret = libelf_registry_verify(registry, modp);
Review Comment:
remove libelf_registry_verify to avoid adding registry argument
##########
libs/libc/dlfcn/lib_dlfcn.h:
##########
@@ -0,0 +1,84 @@
+/****************************************************************************
+ * libs/libc/dlfcn/lib_dlfcn.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __LIBS_LIBC_DLFCN_LIB_DLFCN_H
+#define __LIBS_LIBC_DLFCN_LIB_DLFCN_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+
+#include <nuttx/list.h>
+#include <nuttx/mutex.h>
+#include <nuttx/tls.h>
+
+#ifdef CONFIG_LIBC_ELF
+
+/****************************************************************************
+ * Inline Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: dl_registry_lock
+ *
+ * Description:
+ * Lock the registry of the calling task group, which holds what dlopen()
+ * loads, and return it. The lock is recursive: the constructors of a
+ * library run with it held, and they can call dlopen().
+ *
+ ****************************************************************************/
+
+static inline FAR struct list_node *dl_registry_lock(void)
Review Comment:
should we move to tls.h?
##########
sched/group/group_leave.c:
##########
@@ -80,6 +81,30 @@ static inline void group_release(FAR struct task_group_s
*group)
nxrmutex_destroy(&group->tg_mutex);
+#if defined(CONFIG_LIBC_ELF) && defined(CONFIG_BUILD_FLAT)
+ /* Close what dlopen() left open. No thread of the group runs now, so
+ * the lock is not needed.
+ */
+
+ libelf_removeall(&group->tg_info->ta_modules);
+
+# ifdef CONFIG_BINFMT_LOADABLE
+ /* The rest is used by the program. Give it back to the program before
+ * the list goes with the task information: binfmt_exit() unloads it.
+ */
+
+ if (group->tg_bininfo != NULL)
+ {
+ FAR struct list_node *node;
+
+ while ((node = list_remove_head(&group->tg_info->ta_modules)) != NULL)
+ {
+ list_add_tail(&group->tg_bininfo->libraries, node);
Review Comment:
why need
##########
binfmt/binfmt_execmodule.c:
##########
@@ -190,6 +191,10 @@ int exec_module(FAR struct binary_s *binp,
char name[PATH_MAX];
#endif
FAR void *stackaddr = NULL;
+#if defined(CONFIG_BINFMT_LOADABLE) && defined(CONFIG_LIBC_ELF) && \
Review Comment:
why check CONFIG_LIBC_ELF in the common loader
##########
binfmt/binfmt_execmodule.c:
##########
@@ -190,6 +191,10 @@ int exec_module(FAR struct binary_s *binp,
char name[PATH_MAX];
#endif
FAR void *stackaddr = NULL;
+#if defined(CONFIG_BINFMT_LOADABLE) && defined(CONFIG_LIBC_ELF) && \
+ defined(CONFIG_BUILD_FLAT)
Review Comment:
why need check flat mode? the same code should be used for other mode too
##########
include/nuttx/lib/elf.h:
##########
@@ -776,9 +764,10 @@ FAR void *libelf_insert(FAR const char *filename, FAR
const char *modname);
* called.
*
* Input Parameters:
- * handle - The opaque, non-NULL value returned by a previous successful
- * call to libelf_insert().
- * name - A pointer to the symbol name string.
+ * registry - The registry the module is on.
+ * handle - The opaque, non-NULL value returned by a previous successful
Review Comment:
why need both registry and handle?
##########
libs/libc/elf/elf_bind.c:
##########
@@ -1040,7 +1042,20 @@ static int libelf_relocatedyn(FAR struct module_s *modp,
return ret;
}
- handle = libelf_insertneeded((FAR const char *)loadinfo->iobuffer);
+ /* The library goes on the registry of the module that needs it */
+
+ if (loadinfo->registry == NULL)
Review Comment:
move the check into libelf_insertneeded or libelf_insert
##########
binfmt/elf.c:
##########
@@ -141,6 +141,12 @@ static int elf_loadbinary(FAR struct binary_s *binp,
if (loadinfo.ehdr.e_type == ET_REL || loadinfo.gotsize != 0)
{
+ /* Its DT_NEEDED libraries go on binp->libraries. The task group
+ * does not exist yet. In a flat build, exec_module() gives them to
+ * the new task group.
+ */
+
+ loadinfo.registry = &binp->libraries;
Review Comment:
how about pass ta_modules directly
##########
libs/libc/elf/elf_symbols.c:
##########
@@ -470,7 +470,9 @@ int libelf_symvalue(FAR struct module_s *modp,
exportinfo.modp = modp;
exportinfo.symbol = NULL;
- ret = libelf_registry_foreach(libelf_symcallback,
+ ret = loadinfo->registry == NULL ? OK :
Review Comment:
move all registry check into libef_reigstry_xxx
##########
include/nuttx/tls.h:
##########
@@ -173,6 +173,10 @@ struct task_info_s
size_t ta_envpc; /* Maximum entries of environment array */
size_t ta_envc; /* Number of environment strings */
#endif
+#ifdef CONFIG_LIBC_ELF
Review Comment:
check binfmt?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
