Re: Minor fix to COFF .section change

Re: Minor fix to COFF .section change

[Nick Clifton]

Hi Mark,

: There seems to be another problem in my opinion as a consequence of
: the current default.
: 
: .section .foo,"w"
: .section .foo2,"d"
: 
: produces:
: 
:   3 .foo          00000000  00000000  00000000  00000000  2**2
:                   ALLOC, LOAD, CODE
:   4 .foo2         00000000  00000000  00000000  00000000  2**2
:                   ALLOC, LOAD, DATA
: 
: The !BFD version handles 'w' and 'd' the same way, I think the BFD version 
: should too. Thoughts?

Hmm, well I agree that .foo ought to be a DATA section not a CODE
section, so that in this case they ought to end up with the same
attributes.

Looking at the source in obj-coff.c it seems that "d" type sections
explicitly have SEC_DATA and SEC_LOAD set, whereas "w" sections do
not.  I am not sure why this should be, although I suspect it is
because the m88k does not default to setting these attributes for its
sections.

I assume that your proposed patch fixes this problem as well ?

: > I'm concerned that there may be software out there that may 
: > create a section, expecting it to be executable (the old way) but 
: > finding it's not (the new way).
: 
: I do believe the proposed new default matches the documentation, but
; I see your point. I looked through the GCC config files that define 
: ASM_OUTPUT_SECTION, but it wasn't much help because they all use
: pretty much the same in always adding the attribute so there's never
: a doubt about what's intended. DJGPP should do the same in gcc 3.0.  
: 
: Anyway, as I see it there are several ways to go:
: 
: 1. Go ahead and change the default attribute to match the docs.
: 
: 2. Go with #1, but if it turns out to be a problem for a target, let
:    it default to code.
: 2a. Let a target choose between defaulting to code or to data.
: 
: 3. Recognize .eh_frame (as the .section doc allows) and set SEC_DATA
: for it. 
: 
: My preference is for door #2. Door #3 goes in the direction of 
: automatically setting the right attributes for every special section
: there is.

I like 2/2a too.  Can you try generating a new version of the patch
that creates a new target macro (eg TC_COFF_DEFAULT_SECTION_ATTRIBUTES) 
which if not defined defaults to SEC_LOAD | SEC_DATA, and which then
uses this macro in obj_coff_section().  If you can also generate a
paragraph to go into doc/internals.texi describing this new macro,
that would be great.

Cheers
	Nick

Re: Minor fix to COFF .section change

[Mark E.]

Hi Nick,

> Looking at the source in obj-coff.c it seems that "d" type sections
> explicitly have SEC_DATA and SEC_LOAD set, whereas "w" sections do

> I assume that your proposed patch fixes this problem as well ?

Right. Changing the default will fix this, since "w" is given the default 
attribute.

> that creates a new target macro (eg TC_COFF_DEFAULT_SECTION_ATTRIBUTES) 

>  If you can also generate a
> paragraph to go into doc/internals.texi describing this new macro,

No problem. So I don't waste your time by guessing and guessing wrong, would 
please let me know where in internals.texi the paragraph should go?

Thanks,
Mark

Re: Minor fix to COFF .section change

[Nick Clifton]

Hi Mark,

: > that creates a new target macro (eg TC_COFF_DEFAULT_SECTION_ATTRIBUTES) 
: 
: >  If you can also generate a
: > paragraph to go into doc/internals.texi describing this new macro,
: 
: No problem. So I don't waste your time by guessing and guessing wrong, would 
: please let me know where in internals.texi the paragraph should go?

Sure.  I would suggest adding the entry to the end of the table that
deals with internal macros in the' CPU backend' node (around line 1341
of the gas/doc/internals.texi file).

Cheers
	Nick

Re: Minor fix to COFF .section change

[Mark E.]

There seems to be another problem in my opinion as a consequence of the 
current default.

.section .foo,"w"
.section .foo2,"d"

produces:

  3 .foo          00000000  00000000  00000000  00000000  2**2
                  ALLOC, LOAD, CODE
  4 .foo2         00000000  00000000  00000000  00000000  2**2
                  ALLOC, LOAD, DATA

The !BFD version handles 'w' and 'd' the same way, I think the BFD version 
should too. Thoughts?

Re: Minor fix to COFF .section change

[Mark E.]

Looks like a earlier draft got away from me. The one with 2 and 2a is what I 
meant.

Re: Minor fix to COFF .section change

[Mark E.]

> It seems we would have had this problem before.  Why didn't it fail
> before?

We (as in DJGPP BTW) had this problem with !bfd gas where it thought the 
.eh_frame section contained code and filled the section with nops and 
equivalents. Now we have a similiar problem with bfd gas. 

> I'm concerned that there may be software out there that may
> create a section, expecting it to be executable (the old way) but
> finding it's not (the new way).

I do belive the proposed new default matches the documentation, but I see 
your 
point. I looked through the GCC config files that define ASM_OUTPUT_SECTION, 
but it wasn't much help because they all use pretty much the same in always 
adding the attribute so there's never a doubt about what's intended. DJGPP 
should do the same in gcc 3.0.

Anyway, as I see it there are several ways to go:

1. Go ahead and change the default attribute to match the docs.

2. Keep the current default, but let it be overridable by a target.

3. Recognize .eh_frame (as the .section doc allows) and set SEC_DATA for it.

My preference is for door #1 or door #2. Door #3 goes in the direction of 
automatically setting the right attributes for every special section there 
is.

Re: Minor fix to COFF .section change

[Mark E.]

> It seems we would have had this problem before.  Why didn't it fail
> before?

We (as in DJGPP BTW) had this problem with !bfd gas where it thought the 
.eh_frame section contained code and filled the section with nops and 
equivalents. Now we have a similiar problem with bfd gas. 

> I'm concerned that there may be software out there that may
> create a section, expecting it to be executable (the old way) but
> finding it's not (the new way).

I do belive the proposed new default matches the documentation, but I see 
your point. I looked through the GCC config files that define 
ASM_OUTPUT_SECTION, but it wasn't much help because they all use pretty much 
the same in always adding the attribute so there's never a doubt about what's 
intended. DJGPP should do the same in gcc 3.0.  

Anyway, as I see it there are several ways to go:

1. Go ahead and change the default attribute to match the docs.

2. Go with #1, but if it turns out to be a problem for a target, let it 
default to code.
2a. Let a target choose between defaulting to code or to data.

3. Recognize .eh_frame (as the .section doc allows) and set SEC_DATA for it.

My preference is for door #1 because  or door #2. Door #3 goes in the 
direction of 
automatically setting the right attributes for every special section there 
is.

Re: Minor fix to COFF .section change

[DJ Delorie]

It seems we would have had this problem before.  Why didn't it fail
before?  I'm concerned that there may be software out there that may
create a section, expecting it to be executable (the old way) but
finding it's not (the new way).

Minor fix to COFF .section change

[Mark E.]

Hi guys,
Looks like I found one more reason to post. It turns out SEC_LOAD may not be so good as 
a default. For example, ".section .eh_frame" according to objdump:

ehframe.s:

.section .eh_frame

  3 .eh_frame     00000000  00000000  00000000  00000000  2**2
                  ALLOC, LOAD, CODE

Ouch. Not only does this cause crashes when throwing a C++ exception if the default is 
treated as code, but it's contrary to my reading of the .section docs. I didn't think 
SEC_LOAD by itself would add SEC_ALLOC and SEC_CODE, but looking at styp_to_sec_flags 
and sec_to_styp_flags reveals this to be true. I believe the right thing to do is make 
the default so that SEC_CODE can't be added and guarantee that the section is writable 
according to the docs.

2000-07-01  Mark Elbrecht  <snowball3@bigfoot.com>

	* config/obj-coff.c (obj_coff_section) [BFD_ASSEMBLER]: Make the
	  default section flags 'SEC_LOAD | SEC_DATA' to prevent the section
	  from mistakenly being marked as code when output.

Index: src/gas/config/obj-coff.c
===================================================================
RCS file: /cvs/src/src/gas/config/obj-coff.c,v
retrieving revision 1.26
diff -c -p -r1.26 obj-coff.c
*** obj-coff.c	2000/06/29 23:54:13	1.26
--- obj-coff.c	2000/07/02 01:26:05
*************** obj_coff_section (ignore)
*** 1426,1432 ****
        /* Set section flags for a new section just created by subseg_new.
           Provide a default if no flags were parsed.  */
        if (flags == SEC_NO_FLAGS)
!         flags = SEC_LOAD;
            
  #ifdef COFF_LONG_SECTION_NAMES
        /* Add SEC_LINK_ONCE and SEC_LINK_DUPLICATES_DISCARD to .gnu.linkonce
--- 1426,1432 ----
        /* Set section flags for a new section just created by subseg_new.
           Provide a default if no flags were parsed.  */
        if (flags == SEC_NO_FLAGS)
!         flags = SEC_LOAD | SEC_DATA;
            
  #ifdef COFF_LONG_SECTION_NAMES
        /* Add SEC_LINK_ONCE and SEC_LINK_DUPLICATES_DISCARD to .gnu.linkonce