Use 1U everywhere in elf/elf.h.

  All the bits should be unsigned int in elf/elf.h since
otherwise you can get a warning for using SHF_EXCLUDE
in certain situations.

See:
https://lists.fedorahosted.org/pipermail/elfutils-devel/2015-January/004576.html

I just made them all use 1U for consistency.

No regressions on x86_64.

OK to commit?

Cheers,
Carlos.

2015-03-06  Carlos O'Donell  <carlos@redhat.com>

	* elf.h: Use 1U everywhere.

---

Cheers,
Carlos.
  

On 06/03/15 17:15, Carlos O'Donell wrote:
> All the bits should be unsigned int in elf/elf.h since
> otherwise you can get a warning for using SHF_EXCLUDE
> in certain situations.
> 
> See:
> https://lists.fedorahosted.org/pipermail/elfutils-devel/2015-January/004576.html
> 
> I just made them all use 1U for consistency.

i just want to note here that eg the kernel headers are full of 1<<31
and since it is a common mistake c++14 changed the rules and probably
c will follow:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_463.htm

until then 1U is the right fix
  

On Fri, Mar 6, 2015 at 9:15 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> All the bits should be unsigned int in elf/elf.h since
> otherwise you can get a warning for using SHF_EXCLUDE
> in certain situations.
>
> See:
> https://lists.fedorahosted.org/pipermail/elfutils-devel/2015-January/004576.html
>
> I just made them all use 1U for consistency.
>
> No regressions on x86_64.
>
> OK to commit?
>
> Cheers,
> Carlos.
>
> 2015-03-06  Carlos O'Donell  <carlos@redhat.com>
>
>         * elf.h: Use 1U everywhere.
>

LGTM.
  

It you're going to do it, you should be consistent and cover all the ones
that aren't written using << too.  (Every bare integer constant has type
int.)  Also, if this is important you should write a test for it.  I'm not
sure what is the easiest or best way to verify that a constant's type is
something in particular.  Note that the right type is not necessarily
something like 'unsigned int', but is a type whose signedness matches and
whose width is no larger than, the type of the corresponding struct field.

The test can use some scripting to grovel the full set of macro names to
test, with some hand-written rules for choosing the type (e.g. 'R_*' ->
'Elf32_Word').
  

On Fri, Mar 06, 2015 at 12:15:58PM -0500, Carlos O'Donell wrote:
> All the bits should be unsigned int in elf/elf.h since
> otherwise you can get a warning for using SHF_EXCLUDE
> in certain situations.
> 
> See:
> https://lists.fedorahosted.org/pipermail/elfutils-devel/2015-January/004576.html
> 
> I just made them all use 1U for consistency.
> 
> No regressions on x86_64.
> 
> OK to commit?

Being that the type of all these constants is changed from a signed
type to an unsigned type, there is a risk of regressions if they're
used in comparisons anywhere. I don't claim any such uses exist, but
it's a risk that should probably be explored. If no problems are found
then I prefer your solution of fixing them all.

Rich
  

On 03/06/2015 01:01 PM, Roland McGrath wrote:
> It you're going to do it, you should be consistent and cover all the ones
> that aren't written using << too.  (Every bare integer constant has type
> int.)  Also, if this is important you should write a test for it.  I'm not
> sure what is the easiest or best way to verify that a constant's type is
> something in particular.  Note that the right type is not necessarily
> something like 'unsigned int', but is a type whose signedness matches and
> whose width is no larger than, the type of the corresponding struct field.
> 
> The test can use some scripting to grovel the full set of macro names to
> test, with some hand-written rules for choosing the type (e.g. 'R_*' ->
> 'Elf32_Word').

I would like to write a test case for it, but I can't get anything to
trigger the original reported failure by readelf when running with
-fsanitize=undefined.

Therefore I can't at present even get a reproducer nor write a regression
test, but I know what what we have for SHF_EXCLUDE is wrong.

Anyone else able to create a test case that shows this?

It would obviously require libubsan for `make check`, but that isn't
too bad?

Cheers,
Carlos.
  

> I would like to write a test case for it, but I can't get anything to
> trigger the original reported failure by readelf when running with
> -fsanitize=undefined.
> 
> Therefore I can't at present even get a reproducer nor write a regression
> test, but I know what what we have for SHF_EXCLUDE is wrong.

A regression test for a reported manifest issue is of course worthwhile.
But I was talking about a generic test that verifies at compile time what
the types actually are rather than some particular characteristic of behavior.
  

On 03/06/2015 05:28 PM, Roland McGrath wrote:
>> I would like to write a test case for it, but I can't get anything to
>> trigger the original reported failure by readelf when running with
>> -fsanitize=undefined.
>>
>> Therefore I can't at present even get a reproducer nor write a regression
>> test, but I know what what we have for SHF_EXCLUDE is wrong.
> 
> A regression test for a reported manifest issue is of course worthwhile.
> But I was talking about a generic test that verifies at compile time what
> the types actually are rather than some particular characteristic of behavior.

How?

I mean I could use C++ and the associated RTTI to do the comparison at
runtime and thus produce a detailed analysis of the types.

Is that what you're thinking too?

Cheers,
Carlos.
  

"Carlos O'Donell" <carlos@redhat.com> writes:

> On 03/06/2015 05:28 PM, Roland McGrath wrote:
>>> I would like to write a test case for it, but I can't get anything to
>>> trigger the original reported failure by readelf when running with
>>> -fsanitize=undefined.
>>>
>>> Therefore I can't at present even get a reproducer nor write a regression
>>> test, but I know what what we have for SHF_EXCLUDE is wrong.
>> 
>> A regression test for a reported manifest issue is of course worthwhile.
>> But I was talking about a generic test that verifies at compile time what
>> the types actually are rather than some particular characteristic of behavior.
>
> How?
>
> I mean I could use C++ and the associated RTTI to do the comparison at
> runtime and thus produce a detailed analysis of the types.
>
> Is that what you're thinking too?

Would something like the following be acceptable?  It builds on
known-elf.h/known-elf.awk that I've earlier today proposed for
contribution to elfutils.

	#include <elfutils/known-elf.h>
	#include <elf.h>

	#define TEST_FITS(VALUE, NAME, W)				\
	  _Static_assert ((VALUE) == 0 || __builtin_log2 ((VALUE)) < W,	\
			  NAME " doesn't fit in " #W " bits");

	#define ELF_ONE_KNOWN_AT(NAME, VALUE) TEST_FITS (VALUE, #VALUE, 32) // uint32_t
	ELF_ALL_KNOWN_AT
	#undef ELF_ONE_KNOWN_AT

	#define ELF_ONE_KNOWN_DT(NAME, VALUE) TEST_FITS (VALUE, #VALUE, 32) // Elf32_Sword
	ELF_ALL_KNOWN_DT
	#define ELF_ONE_KNOWN_DT_ARCH(ARCH) ELF_ALL_KNOWN_DT_##ARCH
	ELF_ALL_KNOWN_DT_ARCHES
	#undef ELF_ONE_KNOWN_DT_ARCH
	#undef ELF_ONE_KNOWN_DT

	#define ELF_ONE_KNOWN_ELFOSABI(NAME, VALUE) TEST_FITS (VALUE, #VALUE, 8) // unsigned char
	ELF_ALL_KNOWN_ELFOSABI
	#undef ELF_ONE_KNOWN_ELFOSABI

	#define ELF_ONE_KNOWN_STV(NAME, VALUE) TEST_FITS (VALUE, #VALUE, 2) // unsigned char & 0x3
	ELF_ALL_KNOWN_STV
	#undef ELF_ALL_KNOWN_STV

	// etc...

If I tweak it to admit only 7 bits for ELFOSABI:

	$ gcc ble.c -std=c11 -Wall
	ble.c:5:3: error: static assertion failed: "ELFOSABI_STANDALONE doesn\'t fit in 7 bits"
	   _Static_assert ((VALUE) == 0 || __builtin_log2 ((VALUE)) < W, \
	   ^
	ble.c:19:45: note: in expansion of macro ‘TEST_FITS’
	 #define ELF_ONE_KNOWN_ELFOSABI(NAME, VALUE) TEST_FITS(VALUE, #VALUE, 7) // unsigned char
	                                             ^
	/home/petr/proj/dwgrep/64/libzwerg/known-elf.h:193:3: note: in expansion of macro ‘ELF_ONE_KNOWN_ELFOSABI’
	   ELF_ONE_KNOWN_ELFOSABI (STANDALONE, ELFOSABI_STANDALONE) \
	   ^
	ble.c:20:1: note: in expansion of macro ‘ELF_ALL_KNOWN_ELFOSABI’
	 ELF_ALL_KNOWN_ELFOSABI

The way this would catch the original bug is that GCC refuses to
compile-time evaluate 1 << 31.  I'm not sure, but it seems that's
because it's UB--change it to 1U or 1L and GCC compile-time evaluates it
happily.  So the compilation would fail, indicating that 1 << 31 is not
a compile-time constant.

Porting back to C<11, the static assert can be emulated by the
negative-sized char array thick.  Then the likes of 1 << 31 that are not
compile-time constants lead to actual calls to log2, which we catch as
link errors.  Not as nice as compile-time error, but also works.

If C++ is acceptable, it should be easy to replace the __builtin_log2
and _Static_assert with a bit of (portable, if C++98 is considered that)
template metaprogramming.

Thanks,
Petr
  

On Tue, 24 Mar 2015, Petr Machata wrote:

> > I mean I could use C++ and the associated RTTI to do the comparison at
> > runtime and thus produce a detailed analysis of the types.
> >
> > Is that what you're thinking too?
> 
> Would something like the following be acceptable?  It builds on
> known-elf.h/known-elf.awk that I've earlier today proposed for
> contribution to elfutils.
> 
> 	#include <elfutils/known-elf.h>

I don't think it's a good idea to depend on elfutils on the glibc host (or 
even on the build system).  Tests built for the host should avoid 
depending on headers other than kernel headers and those included in 
glibc.

If a 32-bit signed type were correct, then it would be correct to have 
((int) 0x80000000) (a negative value) as the value of this macro.  I don't 
see testing whether things fit in a certain number of bits as making 
sense.  It makes more sense to test the types of the macros, if we first 
establish what the correct type is.

Testing types does not need to involve C++ / RTTI.  You can just use 
typeof to do a compile-time test for type compatibility (whether through 
multiple declarations of the same identifier, or using 
__builtin_types_compatible_p).

So what's the correct type?  The obvious type is that of the sh_flags 
field, except that sh_flags has different types (Elf32_Word and 
Elf64_Xword) for 32-bit and 64-bit ELF, so clearly the type can't match 
for one of them.  32-bit unsigned seems appropriate to avoid bogus 
sign-extension in the 64-bit case when the high bit is set.
  

Joseph Myers <joseph@codesourcery.com> writes:

> On Tue, 24 Mar 2015, Petr Machata wrote:
>
>> > I mean I could use C++ and the associated RTTI to do the comparison at
>> > runtime and thus produce a detailed analysis of the types.
>> >
>> > Is that what you're thinking too?
>> 
>> Would something like the following be acceptable?  It builds on
>> known-elf.h/known-elf.awk that I've earlier today proposed for
>> contribution to elfutils.
>> 
>> 	#include <elfutils/known-elf.h>
>
> I don't think it's a good idea to depend on elfutils on the glibc host (or 
> even on the build system).  Tests built for the host should avoid 
> depending on headers other than kernel headers and those included in 
> glibc.

Agreed--I'm not sure how to address this, but we have a nice
re-tabulated elf.h version over there, which is useful for task at
hand.

> If a 32-bit signed type were correct, then it would be correct to have
> ((int) 0x80000000) (a negative value) as the value of this macro.  I
> don't see testing whether things fit in a certain number of bits as
> making sense.  It makes more sense to test the types of the macros, if
> we first establish what the correct type is.

Would a test like this make more sense to you?

#define TEST_FITS(VALUE, NAME, TYPE, W)					\
  _Static_assert ((TYPE) (VALUE) == VALUE /*...*/);

Vast majority of constants in elf.h are signed integers, assertion on
__builtin_types_compatible_p would break for any unsigned field.  But
the constants are suitable for initializing the corresponding field.
E.g. consider:

#define ELFOSABI_NONE		0	/* UNIX System V ABI */

The corresponding field is an unsigned char, but the expression here is
a (signed) integer one.  But surely unsigned char blah = ELFOSABI_NONE
is OK and it shouldn't fail?

> Testing types does not need to involve C++ / RTTI.  You can just use 
> typeof to do a compile-time test for type compatibility (whether through 
> multiple declarations of the same identifier, or using 
> __builtin_types_compatible_p).
>
> So what's the correct type?  The obvious type is that of the sh_flags 
> field, except that sh_flags has different types (Elf32_Word and 
> Elf64_Xword) for 32-bit and 64-bit ELF, so clearly the type can't match 
> for one of them.  32-bit unsigned seems appropriate to avoid bogus 
> sign-extension in the 64-bit case when the high bit is set.

Thanks,
Petr
  

On Wed, 25 Mar 2015, Petr Machata wrote:

> > If a 32-bit signed type were correct, then it would be correct to have
> > ((int) 0x80000000) (a negative value) as the value of this macro.  I
> > don't see testing whether things fit in a certain number of bits as
> > making sense.  It makes more sense to test the types of the macros, if
> > we first establish what the correct type is.
> 
> Would a test like this make more sense to you?
> 
> #define TEST_FITS(VALUE, NAME, TYPE, W)					\
>   _Static_assert ((TYPE) (VALUE) == VALUE /*...*/);

You mean to apply this to both Elf32_Word and Elf64_Xword, in the 
sh_flags case, for example?

I think the difficulty is the C rules for implicit type conversions; a 
comparison (uint64_t) -1 == -1 would evaluate to true, for example.  If 
you want to compare values of arbitrary integer types, as a plain integer 
comparison, you need to compare the signs of the two values, ((TYPE) 
(VALUE) < 0) == ((VALUE) < 0), as well as the plain == comparison.
  

On Tue, Mar 24, 2015 at 09:23:55PM +0000, Joseph Myers wrote:
> On Tue, 24 Mar 2015, Petr Machata wrote:
> > Would something like the following be acceptable?  It builds on
> > known-elf.h/known-elf.awk that I've earlier today proposed for
> > contribution to elfutils.
> > 
> > 	#include <elfutils/known-elf.h>
> 
> I don't think it's a good idea to depend on elfutils on the glibc host (or 
> even on the build system).  Tests built for the host should avoid 
> depending on headers other than kernel headers and those included in 
> glibc.

I am sure we can arrange something so that known-elf.awk can be shared or
included into glibc if necessary. It was written to process the elf.h
as is in glibc (elfutils just includes its own copy to make sure it has
the latest definitions, but we don't install it and we want to keep it
in sync with the glibc one - which is why we really want the SHF_EXCLUDE
bug fixed).

> So what's the correct type?  The obvious type is that of the sh_flags 
> field, except that sh_flags has different types (Elf32_Word and 
> Elf64_Xword) for 32-bit and 64-bit ELF, so clearly the type can't match 
> for one of them.  32-bit unsigned seems appropriate to avoid bogus 
> sign-extension in the 64-bit case when the high bit is set.

Would the easiest solution then just be to just use unsigned hex values
for the SHF constants to avoid all the bit shifts?

/* Legal values for sh_flags (section flags).  */

#define SHF_WRITE            0x00000001u /* Writable */
#define SHF_ALLOC            0x00000002u /* Occupies memory during execution */
#define SHF_EXECINSTR        0x00000004u /* Executable */
#define SHF_MERGE            0x00000010u /* Might be merged */
#define SHF_STRINGS          0x00000020u /* Contains nul-terminated strings */
#define SHF_INFO_LINK        0x00000040u /* `sh_info' contains SHT index */
#define SHF_LINK_ORDER       0x00000080u /* Preserve order after combining */
#define SHF_OS_NONCONFORMING 0x00000100u /* Non-standard OS specific handling
                                            required */
#define SHF_GROUP            0x00000200u /* Section is member of a group.  */
#define SHF_TLS              0x00000400u /* Section hold thread-local data.  */
#define SHF_MASKOS           0x0ff00000u /* OS-specific.  */
#define SHF_MASKPROC         0xf0000000u /* Processor-specific */
#define SHF_ORDERED          0x40000000u /* Special ordering requirement
                                            (Solaris).  */
#define SHF_EXCLUDE          0x80000000u /* Section is excluded unless
                                            referenced or allocated (Solaris).*/
  

On 03/24/2015 05:03 PM, Petr Machata wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
> 
>> On 03/06/2015 05:28 PM, Roland McGrath wrote:
>>>> I would like to write a test case for it, but I can't get anything to
>>>> trigger the original reported failure by readelf when running with
>>>> -fsanitize=undefined.
>>>>
>>>> Therefore I can't at present even get a reproducer nor write a regression
>>>> test, but I know what what we have for SHF_EXCLUDE is wrong.
>>>
>>> A regression test for a reported manifest issue is of course worthwhile.
>>> But I was talking about a generic test that verifies at compile time what
>>> the types actually are rather than some particular characteristic of behavior.
>>
>> How?
>>
>> I mean I could use C++ and the associated RTTI to do the comparison at
>> runtime and thus produce a detailed analysis of the types.
>>
>> Is that what you're thinking too?
> 
> Would something like the following be acceptable?  It builds on
> known-elf.h/known-elf.awk that I've earlier today proposed for
> contribution to elfutils.

Sure. 

> The way this would catch the original bug is that GCC refuses to
> compile-time evaluate 1 << 31.  I'm not sure, but it seems that's
> because it's UB--change it to 1U or 1L and GCC compile-time evaluates it
> happily.  So the compilation would fail, indicating that 1 << 31 is not
> a compile-time constant.
> 
> Porting back to C<11, the static assert can be emulated by the
> negative-sized char array thick.  Then the likes of 1 << 31 that are not
> compile-time constants lead to actual calls to log2, which we catch as
> link errors.  Not as nice as compile-time error, but also works.
> 
> If C++ is acceptable, it should be easy to replace the __builtin_log2
> and _Static_assert with a bit of (portable, if C++98 is considered that)
> template metaprogramming.

Any of your solutions looks good to me.

The test itself can be C or C++.

The test can include the header and test whatever it wants.

We support gcc 4.6 or newer for building glibc, so if that works great.

Cheers,
Carlos.
  

On 03/06/2015 07:21 PM, Rich Felker wrote:
> On Fri, Mar 06, 2015 at 12:15:58PM -0500, Carlos O'Donell wrote:
>> All the bits should be unsigned int in elf/elf.h since
>> otherwise you can get a warning for using SHF_EXCLUDE
>> in certain situations.
>>
>> See:
>> https://lists.fedorahosted.org/pipermail/elfutils-devel/2015-January/004576.html
>>
>> I just made them all use 1U for consistency.
>>
>> No regressions on x86_64.
>>
>> OK to commit?
> 
> Being that the type of all these constants is changed from a signed
> type to an unsigned type, there is a risk of regressions if they're
> used in comparisons anywhere. 

This could also trigger format warnings (and build failures with
-Werror), and change C++ overload resolution.

> I don't claim any such uses exist, but
> it's a risk that should probably be explored. If no problems are found
> then I prefer your solution of fixing them all.

Yes, but I think this means that this change needs a NEWS entry and a bug.
  

On 2015-03-06 21:01, Roland McGrath wrote:
> It you're going to do it, you should be consistent and cover all the ones
> that aren't written using << too.   (Every bare integer constant has type
> int.)

It seems several distinct issues are conflated here.

1. How constants are computed. You cannot test for UB after the fact -- 
the harm is already is done. But writing a standalone test for it should 
be easy -- just compile with UBSAN and run the following code:

#include <elf.h>
int main(void) { SHF_EXCLUDE; }

It should output the following message to stderr:

elf-ub.c:2:18: runtime error: left shift of 1 by 31 places cannot be 
represented in type 'int'

IMHO fixing UB is useful independently of the other issues. If the type 
of the constant is a concern its value can be replaced with INT32_MIN 
without changing its type.

2. Deciding which type constants should have. Assuming that all 
constants are positive and their corresponding fields are unsigned and 
given that conversion to unsigned types is well-defined there is 
probably no much problem here. Hence it's not as important as the first 
item.

Further in the thread it's noted that some constants can be used with 
different types (Elf32_Word and Elf64_Xword for sh_flags). Hence using 
the least unsigned type capable of representing the constant is probably ok.

> Also, if this is important you should write a test for it.  I'm not
> sure what is the easiest or best way to verify that a constant's type is
> something in particular.  Note that the right type is not necessarily
> something like 'unsigned int', but is a type whose signedness matches and
> whose width is no larger than, the type of the corresponding struct field.

You can check a constant against a given type:

#define check(x, type) (((x) < 0 || (x) > -1) == ((type)-1 < 0) && 
sizeof(x) <= sizeof(type))

> The test can use some scripting to grovel the full set of macro names to
> test, with some hand-written rules for choosing the type (e.g. 'R_*' ->
> 'Elf32_Word').

It this is possible then it can be used to add explicit casts to the 
corresponding types to definitions of constant.

HTH.