Opened 5 years ago

Last modified 5 years ago

#798 assigned defect

Definitions in abi/include/abi/elf.h are non-conformant

Reported by: Jiri Svoboda Owned by:
Priority: major Milestone:
Component: helenos/unspecified Version: mainline
Keywords: Cc:
Blocker for: Depends on:
See also:

Description

The definitions in abi/include/abi/elf.h are meant to correspond to definitions as specified by SysV ABI. This commit:

commit ca0e838006b1e44ee4570c6851ffbd7a5a132165
Author: Jiří Zárevúcky <zarevucky.jiri@gmail.com>
Date:   Mon Feb 11 16:28:25 2019 +0100

    Convert preprocessor macros in abi/ to C constructs
    
    Preprocessor macros are an obsolete concept and they complicate things.
    They are also completely unnecessary in most circumstances.
    
    This commit changes untyped numeric constants into anonymous enums,
    typed constants into static const variables, and function-like macros
    into functions.

changed macros to enums (which is less harmful) and introduced enum names (also not very harmful) and changed the case of ELF_ST_BIND, ELF_ST_TYPE, ELF_ST_INFO to lowercase (harmful).

Our elf-related headers should be SysV compatible to prevent confusion and compatibility problems. I suggest keeping them as close as to the spec as possible (keep constants as symbolic constants, keep macros as macros), keep identifiers as they should be.

Change History (5)

comment:1 by Jiří Zárevúcky, 5 years ago

I was under the impression we don't care about that, since parts of the header are already nonstandard and confusing from outsider perspective. If compatibility/consistency with other systems is a concern, I'd suggest completely replacing the entire header with a copy of elf.h from a more mature BSD-licensed system.

comment:2 by Jakub Jermář, 5 years ago

I would like to stay away from third-party code as much as possible. Difference in capitalization seems like a minor difference to me (after all, we want to be compatible with the System V ABI and the B stands for binary), but if irrecoverable, we should simply fix our header.

comment:3 by Jakub Jermář, 5 years ago

As a follow-up to the observation that this is about binary compatibility with System V ABI, the standard does not prescribe how an implementation should name ELF-related identifiers and whether they should be macros or enums. True, it introduces its own types, structs and macros, but IMHO that's solely to define the binary ELF format.

In this case I'd argue that a change from macro ELF32_ST_BIND to static inline function elf_st_bind is not confusing and does not violate the standard in any way.

comment:4 by Jiří Zárevúcky, 5 years ago

Personally, I don't have strong feelings about the style of the header other than it's either 100% ours (and thus consistent with the style of the rest of the system), or 100% compatible. Going halfway one direction and halfway the other seems rather strange to me. And if we want the contents of the header to be the same as that of other systems, it makes zero sense to invent our own version with possibly novel bugs, for the sake of some vague third-party-code-phobia.

comment:5 by Jakub Jermář, 5 years ago

In this case there is no reason why our identifiers should be the same as in someone else's system. A system does not become more ABI-compliant if it uses the source code identifiers from the standard. HelenOS should assume the same binary ELF format as the System V ABI defines and that's all.

Note that there even is no such thing as ELF_ST_BIND. That was already us creating useful abstractions above what the standard uses - ELF32_ST_BIND. I could understand the complaint if we named it in a very confusing way, for example if what the standard calls ELF32_ST_BIND we called elf_foo_bar. But this is not the case. Whether its ELF_ST_BIND or elf_st_bind does not really matter. HelenOS can use both: either as a macro or as an inline function.

Note: See TracTickets for help on using tickets.