* [patch/gas]: simplify frag_grow
@ 2011-07-05 17:49 Tristan Gingold
2011-07-06 4:45 ` Alan Modra
2011-07-26 21:14 ` Steve Ellcey
0 siblings, 2 replies; 14+ messages in thread
From: Tristan Gingold @ 2011-07-05 17:49 UTC (permalink / raw)
To: binutils Development
Hi,
the code for frag_grow looks overly complex.
First, if the condition of the first 'if' statement is false, there is no need to execute the second 'if' statement (as this is
the same condition). Thus the second 'if' statement can be moved inside the first one.
The local variable 'n' is not really used.
The 'while' loop can only be executed once. The function 'frag_new' can either succeed or abort via exit (it uses xmalloc to allocate memory as defined by as.h).
Instead of referencing directly the internal obstack field chunk_size, we can use the documented obstack_chunk_size macro.
There is no need to create an empty frag if there was not enough room in the previous one. We can call frag_new only once.
No regression on powerpc-elf.
Is it ok for trunk ?
As the new code is easier to read without a diff, here is the new function body:
frag_grow (unsigned int nchars)
{
if (obstack_room (&frchain_now->frch_obstack) < nchars)
{
long oldc;
long newc;
/* Not enough room in this frag. Close it. */
frag_wane (frag_now);
oldc = obstack_chunk_size (&frchain_now->frch_obstack);
/* Try to allocate a bit more than needed right now. But don't do
this if we would waste too much memory. Especially necessary
for extremely big (like 2GB initialized) frags. */
if (nchars < 0x10000)
newc = 2 * nchars;
else
newc = nchars + 0x10000;
newc += SIZEOF_STRUCT_FRAG;
if (newc <= 0)
as_fatal (_("can't extend frag to %u chars"), nchars);
obstack_chunk_size (&frchain_now->frch_obstack) = newc;
frag_new (0);
obstack_chunk_size (&frchain_now->frch_obstack) = oldc;
}
}
Tristan.
2011-07-05 Tristan Gingold <gingold@adacore.com>
* frags.c (frag_grow): Simplify the code.
diff --git a/gas/frags.c b/gas/frags.c
index b573ce8..02b19ff 100644
--- a/gas/frags.c
+++ b/gas/frags.c
@@ -75,41 +75,41 @@ frag_alloc (struct obstack *ob)
return ptr;
}
-/* Try to augment current frag by nchars chars.
+/* Try to augment current frag by NCHARS chars.
If there is no room, close of the current frag with a ".fill 0"
- and begin a new frag. Unless the new frag has nchars chars available
- do not return. Do not set up any fields of *now_frag. */
+ and begin a new frag. Do not set up any fields of *now_frag. */
void
frag_grow (unsigned int nchars)
{
if (obstack_room (&frchain_now->frch_obstack) < nchars)
{
- unsigned int n;
long oldc;
+ long newc;
+ /* Not enough room in this frag. Close it. */
frag_wane (frag_now);
- frag_new (0);
- oldc = frchain_now->frch_obstack.chunk_size;
+
+ oldc = obstack_chunk_size (&frchain_now->frch_obstack);
+
/* Try to allocate a bit more than needed right now. But don't do
this if we would waste too much memory. Especially necessary
- for extremely big (like 2GB initialized) frags. */
+ for extremely big (like 2GB initialized) frags. */
if (nchars < 0x10000)
- frchain_now->frch_obstack.chunk_size = 2 * nchars;
+ newc = 2 * nchars;
else
- frchain_now->frch_obstack.chunk_size = nchars + 0x10000;
- frchain_now->frch_obstack.chunk_size += SIZEOF_STRUCT_FRAG;
- if (frchain_now->frch_obstack.chunk_size > 0)
- while ((n = obstack_room (&frchain_now->frch_obstack)) < nchars
- && (unsigned long) frchain_now->frch_obstack.chunk_size > nchars)
- {
- frag_wane (frag_now);
- frag_new (0);
- }
- frchain_now->frch_obstack.chunk_size = oldc;
+ newc = nchars + 0x10000;
+ newc += SIZEOF_STRUCT_FRAG;
+
+ if (newc <= 0)
+ as_fatal (_("can't extend frag to %u chars"), nchars);
+
+ obstack_chunk_size (&frchain_now->frch_obstack) = newc;
+
+ frag_new (0);
+
+ obstack_chunk_size (&frchain_now->frch_obstack) = oldc;
}
- if (obstack_room (&frchain_now->frch_obstack) < nchars)
- as_fatal (_("can't extend frag %u chars"), nchars);
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-05 17:49 [patch/gas]: simplify frag_grow Tristan Gingold
@ 2011-07-06 4:45 ` Alan Modra
2011-07-06 7:25 ` Tristan Gingold
2011-07-26 21:14 ` Steve Ellcey
1 sibling, 1 reply; 14+ messages in thread
From: Alan Modra @ 2011-07-06 4:45 UTC (permalink / raw)
To: Tristan Gingold; +Cc: binutils Development
On Tue, Jul 05, 2011 at 02:04:35PM +0200, Tristan Gingold wrote:
I agree with all your reasoning, but I'd like to keep the final check
so that we don't depend on obstack_chunk_alloc being xmalloc.
> - if (obstack_room (&frchain_now->frch_obstack) < nchars)
> - as_fatal (_("can't extend frag %u chars"), nchars);
> }
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-06 4:45 ` Alan Modra
@ 2011-07-06 7:25 ` Tristan Gingold
2011-07-11 12:05 ` Alan Modra
0 siblings, 1 reply; 14+ messages in thread
From: Tristan Gingold @ 2011-07-06 7:25 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils Development
On Jul 6, 2011, at 5:26 AM, Alan Modra wrote:
> On Tue, Jul 05, 2011 at 02:04:35PM +0200, Tristan Gingold wrote:
>
> I agree with all your reasoning, but I'd like to keep the final check
> so that we don't depend on obstack_chunk_alloc being xmalloc.
>
>> - if (obstack_room (&frchain_now->frch_obstack) < nchars)
>> - as_fatal (_("can't extend frag %u chars"), nchars);
>> }
Thank you for the review.
Here is the updated version with this check added.
Tristan.
diff --git a/gas/frags.c b/gas/frags.c
index b573ce8..5caa16c 100644
--- a/gas/frags.c
+++ b/gas/frags.c
@@ -75,41 +75,47 @@ frag_alloc (struct obstack *ob)
return ptr;
}
-/* Try to augment current frag by nchars chars.
+/* Try to augment current frag by NCHARS chars.
If there is no room, close of the current frag with a ".fill 0"
- and begin a new frag. Unless the new frag has nchars chars available
- do not return. Do not set up any fields of *now_frag. */
+ and begin a new frag. Do not set up any fields of *now_frag. */
void
frag_grow (unsigned int nchars)
{
if (obstack_room (&frchain_now->frch_obstack) < nchars)
{
- unsigned int n;
long oldc;
+ long newc;
+ /* Not enough room in this frag. Close it. */
frag_wane (frag_now);
- frag_new (0);
- oldc = frchain_now->frch_obstack.chunk_size;
+
/* Try to allocate a bit more than needed right now. But don't do
this if we would waste too much memory. Especially necessary
- for extremely big (like 2GB initialized) frags. */
+ for extremely big (like 2GB initialized) frags. */
if (nchars < 0x10000)
- frchain_now->frch_obstack.chunk_size = 2 * nchars;
+ newc = 2 * nchars;
else
- frchain_now->frch_obstack.chunk_size = nchars + 0x10000;
- frchain_now->frch_obstack.chunk_size += SIZEOF_STRUCT_FRAG;
- if (frchain_now->frch_obstack.chunk_size > 0)
- while ((n = obstack_room (&frchain_now->frch_obstack)) < nchars
- && (unsigned long) frchain_now->frch_obstack.chunk_size > nchars)
- {
- frag_wane (frag_now);
- frag_new (0);
- }
- frchain_now->frch_obstack.chunk_size = oldc;
+ newc = nchars + 0x10000;
+ newc += SIZEOF_STRUCT_FRAG;
+
+ if (newc <= 0)
+ as_fatal (_("can't extend frag to %u chars"), nchars);
+
+ /* Force to allocate at least NEWC bytes. */
+ oldc = obstack_chunk_size (&frchain_now->frch_obstack);
+ obstack_chunk_size (&frchain_now->frch_obstack) = newc;
+
+ /* Do the real work: create a new frag. */
+ frag_new (0);
+
+ /* Restore the old chunk size. */
+ obstack_chunk_size (&frchain_now->frch_obstack) = oldc;
+
+ /* Make it obvious that we succeed. */
+ if (obstack_room (&frchain_now->frch_obstack) < nchars)
+ as_fatal (_("can't extend frag %u chars"), nchars);
}
- if (obstack_room (&frchain_now->frch_obstack) < nchars)
- as_fatal (_("can't extend frag %u chars"), nchars);
}
/* Call this to close off a completed frag, and start up a new (empty)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-06 7:25 ` Tristan Gingold
@ 2011-07-11 12:05 ` Alan Modra
2011-07-25 14:49 ` Tristan Gingold
0 siblings, 1 reply; 14+ messages in thread
From: Alan Modra @ 2011-07-11 12:05 UTC (permalink / raw)
To: Tristan Gingold; +Cc: binutils Development
On Wed, Jul 06, 2011 at 09:00:29AM +0200, Tristan Gingold wrote:
> + if (newc <= 0)
> + as_fatal (_("can't extend frag to %u chars"), nchars);
Watch out for proliferation of messages. There isn't any reason for a
difference between this error and the later one. (In fact this
message isn't correct since we are extending the frag *by* nchars, not
to a total size of nchars.)
> + /* Force to allocate at least NEWC bytes. */
> + oldc = obstack_chunk_size (&frchain_now->frch_obstack);
> + obstack_chunk_size (&frchain_now->frch_obstack) = newc;
> +
> + /* Do the real work: create a new frag. */
> + frag_new (0);
> +
> + /* Restore the old chunk size. */
> + obstack_chunk_size (&frchain_now->frch_obstack) = oldc;
An alternative to fixing the error message would be to wrap the above
all in "if (newc > 0){}" and dispense with the first as_fatal call.
Your choice. Either way is OK to commit.
> + /* Make it obvious that we succeed. */
> + if (obstack_room (&frchain_now->frch_obstack) < nchars)
> + as_fatal (_("can't extend frag %u chars"), nchars);
> }
> - if (obstack_room (&frchain_now->frch_obstack) < nchars)
> - as_fatal (_("can't extend frag %u chars"), nchars);
> }
>
>
> /* Call this to close off a completed frag, and start up a new (empty)
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-11 12:05 ` Alan Modra
@ 2011-07-25 14:49 ` Tristan Gingold
2011-07-26 20:14 ` Steve Ellcey
0 siblings, 1 reply; 14+ messages in thread
From: Tristan Gingold @ 2011-07-25 14:49 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils Development
On Jul 11, 2011, at 4:05 AM, Alan Modra wrote:
> On Wed, Jul 06, 2011 at 09:00:29AM +0200, Tristan Gingold wrote:
>> + if (newc <= 0)
>> + as_fatal (_("can't extend frag to %u chars"), nchars);
>
> Watch out for proliferation of messages. There isn't any reason for a
> difference between this error and the later one. (In fact this
> message isn't correct since we are extending the frag *by* nchars, not
> to a total size of nchars.)
>
>> + /* Force to allocate at least NEWC bytes. */
>> + oldc = obstack_chunk_size (&frchain_now->frch_obstack);
>> + obstack_chunk_size (&frchain_now->frch_obstack) = newc;
>> +
>> + /* Do the real work: create a new frag. */
>> + frag_new (0);
>> +
>> + /* Restore the old chunk size. */
>> + obstack_chunk_size (&frchain_now->frch_obstack) = oldc;
>
> An alternative to fixing the error message would be to wrap the above
> all in "if (newc > 0){}" and dispense with the first as_fatal call.
> Your choice. Either way is OK to commit.
Thanks. Here is what I am committing (using the later suggestion).
Tristan.
Index: frags.c
===================================================================
RCS file: /cvs/src/src/gas/frags.c,v
retrieving revision 1.26
diff -c -r1.26 frags.c
*** frags.c 11 Sep 2009 15:27:33 -0000 1.26
--- frags.c 25 Jul 2011 13:32:47 -0000
***************
*** 75,115 ****
return ptr;
}
! /* Try to augment current frag by nchars chars.
If there is no room, close of the current frag with a ".fill 0"
! and begin a new frag. Unless the new frag has nchars chars available
! do not return. Do not set up any fields of *now_frag. */
void
frag_grow (unsigned int nchars)
{
if (obstack_room (&frchain_now->frch_obstack) < nchars)
{
- unsigned int n;
long oldc;
frag_wane (frag_now);
! frag_new (0);
! oldc = frchain_now->frch_obstack.chunk_size;
/* Try to allocate a bit more than needed right now. But don't do
this if we would waste too much memory. Especially necessary
! for extremely big (like 2GB initialized) frags. */
if (nchars < 0x10000)
! frchain_now->frch_obstack.chunk_size = 2 * nchars;
else
! frchain_now->frch_obstack.chunk_size = nchars + 0x10000;
! frchain_now->frch_obstack.chunk_size += SIZEOF_STRUCT_FRAG;
! if (frchain_now->frch_obstack.chunk_size > 0)
! while ((n = obstack_room (&frchain_now->frch_obstack)) < nchars
! && (unsigned long) frchain_now->frch_obstack.chunk_size > nchars)
! {
! frag_wane (frag_now);
! frag_new (0);
! }
! frchain_now->frch_obstack.chunk_size = oldc;
}
- if (obstack_room (&frchain_now->frch_obstack) < nchars)
- as_fatal (_("can't extend frag %u chars"), nchars);
}
/* Call this to close off a completed frag, and start up a new (empty)
--- 75,121 ----
return ptr;
}
! /* Try to augment current frag by NCHARS chars.
If there is no room, close of the current frag with a ".fill 0"
! and begin a new frag. Do not set up any fields of *now_frag. */
void
frag_grow (unsigned int nchars)
{
if (obstack_room (&frchain_now->frch_obstack) < nchars)
{
long oldc;
+ long newc;
+ /* Not enough room in this frag. Close it. */
frag_wane (frag_now);
!
/* Try to allocate a bit more than needed right now. But don't do
this if we would waste too much memory. Especially necessary
! for extremely big (like 2GB initialized) frags. */
if (nchars < 0x10000)
! newc = 2 * nchars;
else
! newc = nchars + 0x10000;
! newc += SIZEOF_STRUCT_FRAG;
!
! if (newc > 0)
! {
! /* Force to allocate at least NEWC bytes. */
! oldc = obstack_chunk_size (&frchain_now->frch_obstack);
! obstack_chunk_size (&frchain_now->frch_obstack) = newc;
!
! /* Do the real work: create a new frag. */
! frag_new (0);
!
! /* Restore the old chunk size. */
! obstack_chunk_size (&frchain_now->frch_obstack) = oldc;
! }
!
! /* Check for success (also handles negative values of NEWC). */
! if (obstack_room (&frchain_now->frch_obstack) < nchars)
! as_fatal (_("can't extend frag %u chars"), nchars);
}
}
/* Call this to close off a completed frag, and start up a new (empty)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-25 14:49 ` Tristan Gingold
@ 2011-07-26 20:14 ` Steve Ellcey
0 siblings, 0 replies; 14+ messages in thread
From: Steve Ellcey @ 2011-07-26 20:14 UTC (permalink / raw)
To: binutils, gingold
Tristan,
I am getting a "can't extend frag" error message now during my
GCC bootstrap on IA64 HP-UX. It sounds like this cleanup shouldn't
have actually changed what does and doesn't work in the assembler.
Have you gotten any other reports of problems? I didn't see any
on the mailing list.
I am currently trying to cut down my test case (fold-const.c from GCC)
to a smaller assembly file that reproduces the problem.
Steve Ellcey
sje@cup.hp.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-05 17:49 [patch/gas]: simplify frag_grow Tristan Gingold
2011-07-06 4:45 ` Alan Modra
@ 2011-07-26 21:14 ` Steve Ellcey
2011-07-27 6:56 ` Hans-Peter Nilsson
2011-07-27 11:42 ` [patch/gas]: " Tristan Gingold
1 sibling, 2 replies; 14+ messages in thread
From: Steve Ellcey @ 2011-07-26 21:14 UTC (permalink / raw)
To: Tristan Gingold, binutils
Tristan Gingold wrote:
> The 'while' loop can only be executed once. The function 'frag_new' can
> either succeed or abort via exit (it uses xmalloc to allocate memory as
> defined by as.h).
This doesn't seem to be true. I used the previous version of frag_grow
and put some print statements in frag_grow and I see multiple passes
through the while loop in some calls to frag_grow. If I change the
while to an if (so that it can only be done once) and make no other
changes, my test case fails.
Steve Ellcey
sje@cup.hp.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-26 21:14 ` Steve Ellcey
@ 2011-07-27 6:56 ` Hans-Peter Nilsson
2011-07-27 7:39 ` Tristan Gingold
` (2 more replies)
2011-07-27 11:42 ` [patch/gas]: " Tristan Gingold
1 sibling, 3 replies; 14+ messages in thread
From: Hans-Peter Nilsson @ 2011-07-27 6:56 UTC (permalink / raw)
To: Steve Ellcey; +Cc: Tristan Gingold, binutils
On Tue, 26 Jul 2011, Steve Ellcey wrote:
>
> Tristan Gingold wrote:
>
> > The 'while' loop can only be executed once. The function 'frag_new' can
> > either succeed or abort via exit (it uses xmalloc to allocate memory as
> > defined by as.h).
>
> This doesn't seem to be true. I used the previous version of frag_grow
> and put some print statements in frag_grow and I see multiple passes
> through the while loop in some calls to frag_grow. If I change the
> while to an if (so that it can only be done once) and make no other
> changes, my test case fails.
I'm guessing that bug is also the cause for these regressions
for cris-elf exposed by Tristan's changes:
Running /tmp/hpautotest-binutils/bsrc/src/gas/testsuite/gas/cris/cris.exp ...
FAIL: gas/cris/rd-bkw5
FAIL: gas/cris/rd-bkw5b
FAIL: gas/cris/rd-bkw5pic
FAIL: gas/cris/rd-bkw5v32
FAIL: gas/cris/rd-bkw5v32pic
gas.log (beware, cutnpasted):
../as-new --underscore --em=criself -o dump.o
/tmp/hpautotest-binutils/bsrc/src/gas/testsuite/gas/c
ris/rd-bkw5.s
Executing on host: sh -c {../as-new --underscore --em=criself
-o dump.o /tmp/hpautotest-binutils/bs
rc/src/gas/testsuite/gas/cris/rd-bkw5.s 2>&1} /dev/null gas.out
(timeout = 300)
/tmp/hpautotest-binutils/bsrc/src/gas/testsuite/gas/cris/rd-bkw5.s:
Assembler messages:
/tmp/hpautotest-binutils/bsrc/src/gas/testsuite/gas/cris/rd-bkw5.s:38:
Fatal error: can't extend fra
g 24576 chars
/tmp/hpautotest-binutils/bsrc/src/gas/testsuite/gas/cris/rd-bkw5.s:
Assembler messages:
/tmp/hpautotest-binutils/bsrc/src/gas/testsuite/gas/cris/rd-bkw5.s:38:
Fatal error: can't extend fra
g 24576 chars
/tmp/hpautotest-binutils/cris-axis-elf/gas/testsuite/../../binutils/objdump
-dr dump.o
Executing on host: sh -c
{/tmp/hpautotest-binutils/cris-axis-elf/gas/testsuite/../../binutils/objdum
p -dr dump.o >dump.out 2>gas.stderr} /dev/null (timeout =
300)
/tmp/hpautotest-binutils/cris-axis-elf/gas/testsuite/../../binutils/objdump:
'dump.o': No such file
FAIL: gas/cris/rd-bkw5
I think I'd like to ask Tristan to please revert and investigate
at your own convenience.
brgds, H-P
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-27 6:56 ` Hans-Peter Nilsson
@ 2011-07-27 7:39 ` Tristan Gingold
2011-07-27 10:06 ` Tristan Gingold
2011-07-27 16:08 ` [patch v2/gas]: " Tristan Gingold
2 siblings, 0 replies; 14+ messages in thread
From: Tristan Gingold @ 2011-07-27 7:39 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: Steve Ellcey, binutils
On Jul 27, 2011, at 3:06 AM, Hans-Peter Nilsson wrote:
> On Tue, 26 Jul 2011, Steve Ellcey wrote:
>>
>> Tristan Gingold wrote:
>>
>>> The 'while' loop can only be executed once. The function 'frag_new' can
>>> either succeed or abort via exit (it uses xmalloc to allocate memory as
>>> defined by as.h).
>>
>> This doesn't seem to be true. I used the previous version of frag_grow
>> and put some print statements in frag_grow and I see multiple passes
>> through the while loop in some calls to frag_grow. If I change the
>> while to an if (so that it can only be done once) and make no other
>> changes, my test case fails.
>
> I'm guessing that bug is also the cause for these regressions
> for cris-elf exposed by Tristan's changes:
Ok, I will investigate that.
Tristan.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-27 6:56 ` Hans-Peter Nilsson
2011-07-27 7:39 ` Tristan Gingold
@ 2011-07-27 10:06 ` Tristan Gingold
2011-07-27 16:08 ` [patch v2/gas]: " Tristan Gingold
2 siblings, 0 replies; 14+ messages in thread
From: Tristan Gingold @ 2011-07-27 10:06 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: Steve Ellcey, binutils
On Jul 27, 2011, at 3:06 AM, Hans-Peter Nilsson wrote:
> On Tue, 26 Jul 2011, Steve Ellcey wrote:
>>
>> Tristan Gingold wrote:
>>
>>> The 'while' loop can only be executed once. The function 'frag_new' can
>>> either succeed or abort via exit (it uses xmalloc to allocate memory as
>>> defined by as.h).
>>
>> This doesn't seem to be true. I used the previous version of frag_grow
>> and put some print statements in frag_grow and I see multiple passes
>> through the while loop in some calls to frag_grow. If I change the
>> while to an if (so that it can only be done once) and make no other
>> changes, my test case fails.
>
> I'm guessing that bug is also the cause for these regressions
> for cris-elf exposed by Tristan's changes:
[...]
>
> I think I'd like to ask Tristan to please revert and investigate
> at your own convenience.
I can reproduce this issue (but only when using a 32bits compiler).
Patch reverting and investigation started.
Thank you for catching that.
Tristan.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch/gas]: simplify frag_grow
2011-07-26 21:14 ` Steve Ellcey
2011-07-27 6:56 ` Hans-Peter Nilsson
@ 2011-07-27 11:42 ` Tristan Gingold
1 sibling, 0 replies; 14+ messages in thread
From: Tristan Gingold @ 2011-07-27 11:42 UTC (permalink / raw)
To: Steve Ellcey; +Cc: binutils
On Jul 26, 2011, at 10:13 PM, Steve Ellcey wrote:
>
> Tristan Gingold wrote:
>
>> The 'while' loop can only be executed once. The function 'frag_new' can
>> either succeed or abort via exit (it uses xmalloc to allocate memory as
>> defined by as.h).
>
> This doesn't seem to be true. I used the previous version of frag_grow
> and put some print statements in frag_grow and I see multiple passes
> through the while loop in some calls to frag_grow. If I change the
> while to an if (so that it can only be done once) and make no other
> changes, my test case fails.
You're correct. frag_new doesn't necessary calls xmalloc if there is enough room in the current chunk. Hence the bug.
I will rework this patch. I am not very happy with the fact that we can create empty frags until one is big enough.
Tristan.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [patch v2/gas]: simplify frag_grow
2011-07-27 6:56 ` Hans-Peter Nilsson
2011-07-27 7:39 ` Tristan Gingold
2011-07-27 10:06 ` Tristan Gingold
@ 2011-07-27 16:08 ` Tristan Gingold
2011-07-29 6:01 ` Alan Modra
2 siblings, 1 reply; 14+ messages in thread
From: Tristan Gingold @ 2011-07-27 16:08 UTC (permalink / raw)
To: binutils Development; +Cc: Steve Ellcey, Hans-Peter Nilsson, Alan Modra
Hi,
the issue in the first version of the patch is that frag_new() doesn't necessary allocates memory as it can reuse the current chunk.
So this patch keeps the while loop around frag_new().
So this second version of the patch is less radical than the first one, and I still find the loop unpleasant as it can create empty frags.
But I think this patch makes the code easier to read, still remove unused variables, use obstack_chunk_size instead of accessing directly private fields, avoids an useless 'if' when there is enough room and finally add some comments.
No regressions for cris-elf (which has some nice testcases for broken-words).
Ok for trunk ?
Tristan.
2011-07-05 Tristan Gingold <gingold@adacore.com>
* frags.c (frag_grow): Simplify the code.
diff --git a/gas/frags.c b/gas/frags.c
index 17110bb..0457f66 100644
--- a/gas/frags.c
+++ b/gas/frags.c
@@ -85,31 +85,38 @@ frag_grow (unsigned int nchars)
{
if (obstack_room (&frchain_now->frch_obstack) < nchars)
{
- unsigned int n;
long oldc;
+ long newc;
- frag_wane (frag_now);
- frag_new (0);
- oldc = frchain_now->frch_obstack.chunk_size;
/* Try to allocate a bit more than needed right now. But don't do
this if we would waste too much memory. Especially necessary
- for extremely big (like 2GB initialized) frags. */
+ for extremely big (like 2GB initialized) frags. */
if (nchars < 0x10000)
- frchain_now->frch_obstack.chunk_size = 2 * nchars;
+ newc = 2 * nchars;
else
- frchain_now->frch_obstack.chunk_size = nchars + 0x10000;
- frchain_now->frch_obstack.chunk_size += SIZEOF_STRUCT_FRAG;
- if (frchain_now->frch_obstack.chunk_size > 0)
- while ((n = obstack_room (&frchain_now->frch_obstack)) < nchars
- && (unsigned long) frchain_now->frch_obstack.chunk_size > nchars)
- {
- frag_wane (frag_now);
- frag_new (0);
- }
- frchain_now->frch_obstack.chunk_size = oldc;
+ newc = nchars + 0x10000;
+ newc += SIZEOF_STRUCT_FRAG;
+
+ /* Check for possible overflow. */
+ if (newc < 0)
+ as_fatal (_("can't extend frag %u chars"), nchars);
+
+ /* Force to allocate at least NEWC bytes. */
+ oldc = obstack_chunk_size (&frchain_now->frch_obstack);
+ obstack_chunk_size (&frchain_now->frch_obstack) = newc;
+
+ while (obstack_room (&frchain_now->frch_obstack) < nchars)
+ {
+ /* Not enough room in this frag. Close it and start a new one.
+ This must be done in a loop because the created frag may not
+ be big enough if the current obstack chunk is used. */
+ frag_wane (frag_now);
+ frag_new (0);
+ }
+
+ /* Restore the old chunk size. */
+ obstack_chunk_size (&frchain_now->frch_obstack) = oldc;
}
- if (obstack_room (&frchain_now->frch_obstack) < nchars)
- as_fatal (_("can't extend frag %u chars"), nchars);
}
/* Call this to close off a completed frag, and start up a new (empty)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch v2/gas]: simplify frag_grow
2011-07-27 16:08 ` [patch v2/gas]: " Tristan Gingold
@ 2011-07-29 6:01 ` Alan Modra
2011-08-01 8:06 ` Tristan Gingold
0 siblings, 1 reply; 14+ messages in thread
From: Alan Modra @ 2011-07-29 6:01 UTC (permalink / raw)
To: Tristan Gingold; +Cc: binutils Development, Steve Ellcey, Hans-Peter Nilsson
On Wed, Jul 27, 2011 at 03:19:51PM +0200, Tristan Gingold wrote:
> * frags.c (frag_grow): Simplify the code.
OK.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [patch v2/gas]: simplify frag_grow
2011-07-29 6:01 ` Alan Modra
@ 2011-08-01 8:06 ` Tristan Gingold
0 siblings, 0 replies; 14+ messages in thread
From: Tristan Gingold @ 2011-08-01 8:06 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils Development, Steve Ellcey, Hans-Peter Nilsson
On Jul 29, 2011, at 5:20 AM, Alan Modra wrote:
> On Wed, Jul 27, 2011 at 03:19:51PM +0200, Tristan Gingold wrote:
>> * frags.c (frag_grow): Simplify the code.
>
> OK.
Thanks, committed.
I checked that cris-elf is ok.
Tristan.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-08-01 8:06 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-05 17:49 [patch/gas]: simplify frag_grow Tristan Gingold
2011-07-06 4:45 ` Alan Modra
2011-07-06 7:25 ` Tristan Gingold
2011-07-11 12:05 ` Alan Modra
2011-07-25 14:49 ` Tristan Gingold
2011-07-26 20:14 ` Steve Ellcey
2011-07-26 21:14 ` Steve Ellcey
2011-07-27 6:56 ` Hans-Peter Nilsson
2011-07-27 7:39 ` Tristan Gingold
2011-07-27 10:06 ` Tristan Gingold
2011-07-27 16:08 ` [patch v2/gas]: " Tristan Gingold
2011-07-29 6:01 ` Alan Modra
2011-08-01 8:06 ` Tristan Gingold
2011-07-27 11:42 ` [patch/gas]: " Tristan Gingold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).