[review] Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x

[review] Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x

[Florian Weimer (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/470
......................................................................

Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x

This will allow changes in dependency processing during non-lazy
binding, for more precise processing of NODELETE objects: During
initial relocation in dlopen, the fate of NODELETE objects is still
unclear, so objects which are depended upon by NODELETE objects
cannot immediately be marked as NODELETE.

Change-Id: Ic7b94a3f7c4719a00ca8e6018088567824da0658
---
M elf/dl-reloc.c
M sysdeps/generic/ldsodefs.h
2 files changed, 5 insertions(+), 1 deletion(-)



diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
index 725a074..7f201fe 100644
--- a/elf/dl-reloc.c
+++ b/elf/dl-reloc.c
@@ -244,7 +244,8 @@
 	       v = (version);						      \
 	     _lr = _dl_lookup_symbol_x (strtab + (*ref)->st_name, l, (ref),   \
 					scope, v, _tc,			      \
-					DL_LOOKUP_ADD_DEPENDENCY, NULL);      \
+					DL_LOOKUP_ADD_DEPENDENCY	      \
+					| DL_LOOKUP_FOR_RELOCATE, NULL);      \
 	     l->l_lookup_cache.ret = (*ref);				      \
 	     l->l_lookup_cache.value = _lr; }))				      \
      : l)
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index c546757..2d9c6a0 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -923,6 +923,9 @@
     DL_LOOKUP_RETURN_NEWEST = 2,
     /* Set if dl_lookup* called with GSCOPE lock held.  */
     DL_LOOKUP_GSCOPE_LOCK = 4,
+    /* Set if dl_lookup is called for non-lazy relocation processing
+       from _dl_relocate_object in elf/dl-reloc.c.  */
+    DL_LOOKUP_FOR_RELOCATE = 8,
   };
 
 /* Lookup versioned symbol.  */

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic7b94a3f7c4719a00ca8e6018088567824da0658
Gerrit-Change-Number: 470
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: newchange

[review v2] Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x

[Florian Weimer (Code Review)]

Florian Weimer has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/470
......................................................................


Patch Set 2:

This change is ready for review.


-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic7b94a3f7c4719a00ca8e6018088567824da0658
Gerrit-Change-Number: 470
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Wed, 13 Nov 2019 12:57:08 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v3] Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x

[Carlos O'Donell (Code Review)]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/470
......................................................................


Patch Set 3: Code-Review+2

(2 comments)

This looks good to me. We can add and remove any number of bits we need to fix NODELETE tracking. The adding of the bit in RESOLVE_MAP is as good a place as any to put this kind of processing.

OK for master (I reviewed this one out of order because it was easy).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

| --- elf/dl-reloc.c
| +++ elf/dl-reloc.c
| @@ -240,17 +240,18 @@ #define RESOLVE_MAP(ref, version, r_type) \
|  	     l->l_lookup_cache.type_class = _tc;			      \
|  	     l->l_lookup_cache.sym = (*ref);				      \
|  	     const struct r_found_version *v = NULL;			      \
|  	     if ((version) != NULL && (version)->hash != 0)		      \
|  	       v = (version);						      \
|  	     _lr = _dl_lookup_symbol_x (strtab + (*ref)->st_name, l, (ref),   \
|  					scope, v, _tc,			      \
| -					DL_LOOKUP_ADD_DEPENDENCY, NULL);      \
| +					DL_LOOKUP_ADD_DEPENDENCY	      \
| +					| DL_LOOKUP_FOR_RELOCATE, NULL);      \

PS3, Line 248:

OK. Add a new bit for tracking this and set it as part of RESOLVE_MAP
call during relocation processing.

|  	     l->l_lookup_cache.ret = (*ref);				      \
|  	     l->l_lookup_cache.value = _lr; }))				      \
|       : l)
|  
|  #include "dynamic-link.h"
|  
|      ELF_DYNAMIC_RELOCATE (l, lazy, consider_profiling, skip_ifunc);
|  
|  #ifndef PROF
| --- sysdeps/generic/ldsodefs.h
| +++ sysdeps/generic/ldsodefs.h
| @@ -920,16 +920,19 @@ enum
|      /* If necessary add dependency between user and provider object.  */
|      DL_LOOKUP_ADD_DEPENDENCY = 1,
|      /* Return most recent version instead of default version for
|         unversioned lookup.  */
|      DL_LOOKUP_RETURN_NEWEST = 2,
|      /* Set if dl_lookup* called with GSCOPE lock held.  */
|      DL_LOOKUP_GSCOPE_LOCK = 4,
| +    /* Set if dl_lookup is called for non-lazy relocation processing
| +       from _dl_relocate_object in elf/dl-reloc.c.  */
| +    DL_LOOKUP_FOR_RELOCATE = 8,

PS3, Line 929:

OK. Define the bit.

|    };
|  
|  /* Lookup versioned symbol.  */
|  extern lookup_t _dl_lookup_symbol_x (const char *undef,
|  				     struct link_map *undef_map,
|  				     const ElfW(Sym) **sym,
|  				     struct r_scope_elem *symbol_scope[],
|  				     const struct r_found_version *version,
|  				     int type_class, int flags,

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic7b94a3f7c4719a00ca8e6018088567824da0658
Gerrit-Change-Number: 470
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 12:23:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[pushed] Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/470
......................................................................

Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x

This will allow changes in dependency processing during non-lazy
binding, for more precise processing of NODELETE objects: During
initial relocation in dlopen, the fate of NODELETE objects is still
unclear, so objects which are depended upon by NODELETE objects
cannot immediately be marked as NODELETE.

Change-Id: Ic7b94a3f7c4719a00ca8e6018088567824da0658
---
M elf/dl-reloc.c
M sysdeps/generic/ldsodefs.h
2 files changed, 5 insertions(+), 1 deletion(-)

Approvals:
  Carlos O'Donell: Looks good to me, approved


diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
index 725a074..7f201fe 100644
--- a/elf/dl-reloc.c
+++ b/elf/dl-reloc.c
@@ -244,7 +244,8 @@
 	       v = (version);						      \
 	     _lr = _dl_lookup_symbol_x (strtab + (*ref)->st_name, l, (ref),   \
 					scope, v, _tc,			      \
-					DL_LOOKUP_ADD_DEPENDENCY, NULL);      \
+					DL_LOOKUP_ADD_DEPENDENCY	      \
+					| DL_LOOKUP_FOR_RELOCATE, NULL);      \
 	     l->l_lookup_cache.ret = (*ref);				      \
 	     l->l_lookup_cache.value = _lr; }))				      \
      : l)
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 891049d..9737780 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -917,6 +917,9 @@
     DL_LOOKUP_RETURN_NEWEST = 2,
     /* Set if dl_lookup* called with GSCOPE lock held.  */
     DL_LOOKUP_GSCOPE_LOCK = 4,
+    /* Set if dl_lookup is called for non-lazy relocation processing
+       from _dl_relocate_object in elf/dl-reloc.c.  */
+    DL_LOOKUP_FOR_RELOCATE = 8,
   };
 
 /* Lookup versioned symbol.  */

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic7b94a3f7c4719a00ca8e6018088567824da0658
Gerrit-Change-Number: 470
Gerrit-PatchSet: 4
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: merged