diff mbox

[aarch64] Fix glibc.tune.cpu tunable handling

Message ID 1504294850.3182.66.camel@cavium.com
State New, archived
Headers

Commit Message

Steve Ellcey Sept. 1, 2017, 7:40 p.m. UTC 
  While working on a gcc patch I realized that the glibc tunable
'glibc.tune.cpu' was not working on aarch64.  I tracked it down to
get_midr_from_mcpu and at first I thought I should remove the '== 0'
from the if statement because tunable_is_name returns true/false, not
-1/0/1 like strcmp.  But then I realized we shouldn't be using
tunable_is_name at all because we are comparing two null terminated
strings and tunable_is_name is trying to compare a null terminated
string with a '=' terminated string which is not what we have.

I tested this by running a program that calls memcpy and checking what
memcpy gets run after setting glibc.tune.cpu to generic, thunderx,
and falkor.

OK to checkin?

Steve Ellcey
sellcey@cavium.com


2017-09-01  Steve Ellcey  <sellcey@cavium.com>

	* sysdeps/unix/sysv/linux/aarch64/cpu-features.c (get_midr_from_mcpu):
	Use strcmp instead of tunable_is_name.

Comments

Siddhesh Poyarekar Sept. 4, 2017, 5:37 a.m. UTC | #1 
On Saturday 02 September 2017 01:10 AM, Steve Ellcey wrote:
> While working on a gcc patch I realized that the glibc tunable
> 'glibc.tune.cpu' was not working on aarch64.  I tracked it down to
> get_midr_from_mcpu and at first I thought I should remove the '== 0'
> from the if statement because tunable_is_name returns true/false, not
> -1/0/1 like strcmp.  But then I realized we shouldn't be using
> tunable_is_name at all because we are comparing two null terminated
> strings and tunable_is_name is trying to compare a null terminated
> string with a '=' terminated string which is not what we have.
> 
> I tested this by running a program that calls memcpy and checking what
> memcpy gets run after setting glibc.tune.cpu to generic, thunderx,
> and falkor.

Right, tunable_is_name is the wrong function to call but using strcmp
there may not be safe.  Please verify that it does not go through a PLT,
i.e. it calls the ld.so version at all times.

Siddhesh
Steve Ellcey Sept. 5, 2017, 3:53 p.m. UTC | #2 
On Mon, 2017-09-04 at 11:07 +0530, Siddhesh Poyarekar wrote:
> 
> Right, tunable_is_name is the wrong function to call but using strcmp
> there may not be safe.  Please verify that it does not go through a PLT,
> i.e. it calls the ld.so version at all times.
> 
> Siddhesh

I am not sure I know how to do this.  If I look at csu/libc-start.o,
where, after inlining, this code shows up I see a reference to "strcmp"
and not to "__GI_strcmp" that I see in some other places.  Does that
mean I am going through the PLT?  If so, what do I do about it?  I also
see a new undefined reference to strcmp from elf/dl-sysdep.os, but that
file was already referencing "strlen", so maybe it doesn't matter
there.

Steve Ellcey
sellcey@cavium.com
Siddhesh Poyarekar Sept. 6, 2017, 6:23 a.m. UTC | #3 
On Tuesday 05 September 2017 09:23 PM, Steve Ellcey wrote:
> I am not sure I know how to do this.  If I look at csu/libc-start.o,
> where, after inlining, this code shows up I see a reference to "strcmp"
> and not to "__GI_strcmp" that I see in some other places.  Does that
> mean I am going through the PLT?  If so, what do I do about it?  I also
> see a new undefined reference to strcmp from elf/dl-sysdep.os, but that
> file was already referencing "strlen", so maybe it doesn't matter
> there.

When you disassemble ld.so you should see (1) a definition of strcmp and
(2) the call site should have a call to strcmp and not to strcmp@plt.

Siddhesh
Steve Ellcey Sept. 6, 2017, 3:28 p.m. UTC | #4 
On Wed, 2017-09-06 at 11:53 +0530, Siddhesh Poyarekar wrote:
> On Tuesday 05 September 2017 09:23 PM, Steve Ellcey wrote:
> > 
> > I am not sure I know how to do this.  If I look at csu/libc-start.o,
> > where, after inlining, this code shows up I see a reference to "strcmp"
> > and not to "__GI_strcmp" that I see in some other places.  Does that
> > mean I am going through the PLT?  If so, what do I do about it?  I also
> > see a new undefined reference to strcmp from elf/dl-sysdep.os, but that
> > file was already referencing "strlen", so maybe it doesn't matter
> > there.
> When you disassemble ld.so you should see (1) a definition of strcmp and
> (2) the call site should have a call to strcmp and not to strcmp@plt.
> 
> Siddhesh

OK, I disassembled ld.so with 'objdump -d' and the calls are to
'strcmp' and not to 'strcmp@plt'.  I see quite a few of them so
it looks like we were already using strcmp in other places.

I also see the definition of strcmp in ld.so.

Steve Ellcey
sellcey@cavium.com
Siddhesh Poyarekar Sept. 8, 2017, 8:16 a.m. UTC | #5 
On Wednesday 06 September 2017 08:58 PM, Steve Ellcey wrote:
> OK, I disassembled ld.so with 'objdump -d' and the calls are to
> 'strcmp' and not to 'strcmp@plt'.  I see quite a few of them so
> it looks like we were already using strcmp in other places.
> 
> I also see the definition of strcmp in ld.so.

Looks good then.

Thanks,
Siddhesh
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/
linux/aarch64/cpu-features.c
index 18f5e60..0c7e13f 100644
--- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
+++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
@@ -37,7 +37,7 @@  static uint64_t
 get_midr_from_mcpu (const char *mcpu)
 {
   for (int i = 0; i < sizeof (cpu_list) / sizeof (struct cpu_list); i++)
-    if (tunable_is_name (mcpu, cpu_list[i].name) == 0)
+    if (strcmp (mcpu, cpu_list[i].name) == 0)
       return cpu_list[i].midr;
 
   return UINT64_MAX;