Add .size directives to x86_64 start.S, and possibly more
diff mbox

Message ID CAEG7qUyQPNG1EtSwWSC77vUm+PZXUpDBtm5obab+Y5aa9Vv4-A@mail.gmail.com
State New
Headers show

Commit Message

Sterling Augustine Nov. 21, 2014, 6:44 p.m. UTC
Hello,

sysdeps/x86_64/start.S doesn't have a .size elf directive for _start.
This tripped up some analysis I was doing.

This patch is the straightforward fix. Would you be interested in a
similar fix for the ~150 assembly files inside x86_64 with similar
issues?

Sterling

ChangeLog

2014-11-21  Sterling Augustine  <saugustine@google.com>

* sysdeps/x86_64/start.S: Add .size directive.

Comments

H.J. Lu Nov. 21, 2014, 6:52 p.m. UTC | #1
On Fri, Nov 21, 2014 at 10:44 AM, Sterling Augustine
<saugustine@google.com> wrote:
> Hello,
>
> sysdeps/x86_64/start.S doesn't have a .size elf directive for _start.
> This tripped up some analysis I was doing.
>
> This patch is the straightforward fix. Would you be interested in a
> similar fix for the ~150 assembly files inside x86_64 with similar
> issues?
>
> Sterling
>
> ChangeLog
>
> 2014-11-21  Sterling Augustine  <saugustine@google.com>
>
> * sysdeps/x86_64/start.S: Add .size directive.
>
>
> diff --git a/sysdeps/x86_64/start.S b/sysdeps/x86_64/start.S
> index e3d4ff8..5106bd0 100644
> --- a/sysdeps/x86_64/start.S
> +++ b/sysdeps/x86_64/start.S
> @@ -124,6 +124,7 @@ _start:
>
>   hlt /* Crash if somehow `exit' does return. */
>   cfi_endproc
> + .size _start, .-_start
>
>  /* Define a symbol for the first piece of initialized data.  */
>   .data

Why not use ENTRY/END macros?
Sterling Augustine Nov. 21, 2014, 7:02 p.m. UTC | #2
On Fri, Nov 21, 2014 at 10:52 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Nov 21, 2014 at 10:44 AM, Sterling Augustine
> <saugustine@google.com> wrote:
>> Hello,
>>
>> sysdeps/x86_64/start.S doesn't have a .size elf directive for _start.
>> This tripped up some analysis I was doing.
>>
>> This patch is the straightforward fix. Would you be interested in a
>> similar fix for the ~150 assembly files inside x86_64 with similar
>> issues?
>>
>> Sterling
>>
>> ChangeLog
>>
>> 2014-11-21  Sterling Augustine  <saugustine@google.com>
>>
>> * sysdeps/x86_64/start.S: Add .size directive.
>>
>>
>> diff --git a/sysdeps/x86_64/start.S b/sysdeps/x86_64/start.S
>> index e3d4ff8..5106bd0 100644
>> --- a/sysdeps/x86_64/start.S
>> +++ b/sysdeps/x86_64/start.S
>> @@ -124,6 +124,7 @@ _start:
>>
>>   hlt /* Crash if somehow `exit' does return. */
>>   cfi_endproc
>> + .size _start, .-_start
>>
>>  /* Define a symbol for the first piece of initialized data.  */
>>   .data
>
> Why not use ENTRY/END macros?

I'm hazy here, but I don't think things are sufficiently initialized
by _start to call mcount, which ENTRY does. You are right though, in
that most of the other files do use END, and so the cleanup isn't as
big as I thought.

It probably doesn't make sense to use an END without a corresponding
ENTRY in start.S, but I'm fine with whatever.
H.J. Lu Nov. 21, 2014, 7:06 p.m. UTC | #3
On Fri, Nov 21, 2014 at 11:02 AM, Sterling Augustine
<saugustine@google.com> wrote:
> On Fri, Nov 21, 2014 at 10:52 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Fri, Nov 21, 2014 at 10:44 AM, Sterling Augustine
>> <saugustine@google.com> wrote:
>>> Hello,
>>>
>>> sysdeps/x86_64/start.S doesn't have a .size elf directive for _start.
>>> This tripped up some analysis I was doing.
>>>
>>> This patch is the straightforward fix. Would you be interested in a
>>> similar fix for the ~150 assembly files inside x86_64 with similar
>>> issues?
>>>
>>> Sterling
>>>
>>> ChangeLog
>>>
>>> 2014-11-21  Sterling Augustine  <saugustine@google.com>
>>>
>>> * sysdeps/x86_64/start.S: Add .size directive.
>>>
>>>
>>> diff --git a/sysdeps/x86_64/start.S b/sysdeps/x86_64/start.S
>>> index e3d4ff8..5106bd0 100644
>>> --- a/sysdeps/x86_64/start.S
>>> +++ b/sysdeps/x86_64/start.S
>>> @@ -124,6 +124,7 @@ _start:
>>>
>>>   hlt /* Crash if somehow `exit' does return. */
>>>   cfi_endproc
>>> + .size _start, .-_start
>>>
>>>  /* Define a symbol for the first piece of initialized data.  */
>>>   .data
>>
>> Why not use ENTRY/END macros?
>
> I'm hazy here, but I don't think things are sufficiently initialized
> by _start to call mcount, which ENTRY does. You are right though, in
> that most of the other files do use END, and so the cleanup isn't as
> big as I thought.
>
> It probably doesn't make sense to use an END without a corresponding
> ENTRY in start.S, but I'm fine with whatever.

We can add

# define ENTRY_NO_MCOUNT
...
#define ENRTY ...
     ENTRY_NO_MCOUNT
     CALL_MCOUNT

Then we can use ENTRY_NO_MCOUNT if needed.

H.J.
Andreas Schwab Nov. 21, 2014, 7:57 p.m. UTC | #4
Sterling Augustine <saugustine@google.com> writes:

> I'm hazy here, but I don't think things are sufficiently initialized
> by _start to call mcount, which ENTRY does.

start doesn't have a .po variant, so it doesn't matter.

Andreas.
Sterling Augustine Nov. 24, 2014, 6:43 p.m. UTC | #5
On Fri, Nov 21, 2014 at 11:57 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Sterling Augustine <saugustine@google.com> writes:
>
>> I'm hazy here, but I don't think things are sufficiently initialized
>> by _start to call mcount, which ENTRY does.
>
> start doesn't have a .po variant, so it doesn't matter.

So should I follow HJ's approach, or just use an plain ENTRY macro?
H.J. Lu Nov. 24, 2014, 6:50 p.m. UTC | #6
On Mon, Nov 24, 2014 at 10:43 AM, Sterling Augustine
<saugustine@google.com> wrote:
> On Fri, Nov 21, 2014 at 11:57 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> Sterling Augustine <saugustine@google.com> writes:
>>
>>> I'm hazy here, but I don't think things are sufficiently initialized
>>> by _start to call mcount, which ENTRY does.
>>
>> start doesn't have a .po variant, so it doesn't matter.
>
> So should I follow HJ's approach, or just use an plain ENTRY macro?

Plain ENTRY is preferred if it works.

Thanks.

Patch
diff mbox

diff --git a/sysdeps/x86_64/start.S b/sysdeps/x86_64/start.S
index e3d4ff8..5106bd0 100644
--- a/sysdeps/x86_64/start.S
+++ b/sysdeps/x86_64/start.S
@@ -124,6 +124,7 @@  _start:

  hlt /* Crash if somehow `exit' does return. */
  cfi_endproc
+ .size _start, .-_start

 /* Define a symbol for the first piece of initialized data.  */
  .data