7
\$\begingroup\$

Part 1

I've followed some of the suggestions:

  • Adding newlines to error messages
  • Implementing options (I decided to use boost instead of getopt)
  • Using strerror for fstream failure
  • Allow multiple files to be compiled (to produce one binary - I wasn't sure if the suggestion meant one binary for each file)

Things I did not implement yet:

  • Localization
  • Reading code from standard input instead of files
  • Allowing the user to choose which architecture and platform the binary should be for (trivial)

Things I'm looking for in the review:

  • Readability. Especially on the program structure and comments

  • Whether or not it is intuitive for somebody who's used to a compiler like GCC. For example, I added --silent as a suggestion from the previous question, but it's not an option that I'm too familiar with. Although it DOES save people the trouble of piping strerr to /dev/null/

  • Should the error for chmod be handled at the caller or inside set_executable_permissions?

Example run:

% ./nothing
nothing: fatal error: no input files
compilation terminated.
% ./nothing --help
usage: ./nothing [OPTIONS] <input-file>...
Allowed options:
  --help                            produce help message
  --silent                          suppress error messages
  -o [ --output-file ] arg (=a.out) output file
% ./nothing asdf
nothing: error: asdf: No such file or directory
nothing: fatal error: no input files
compilation terminated.
% ./nothing --silent asdf
% touch empty.not
% ./nothing -o my_program empty.not
% objdump -d ./my_program

./my_program:     file format elf64-x86-64

Disassembly of section .text:

0000000008048000 <.text>:
 8048000:   bb 00 00 00 00          mov    $0x0,%ebx
 8048005:   b8 01 00 00 00          mov    $0x1,%eax
 804800a:   cd 80                   int    $0x80

And the code dump:

/*
 * A 'Nothing' compiler
 * <http://www.turtle.dds.nl/nothing/>
 * September 2010, RoPe Development Inc.
 *
 * Author: authorname
 */
#include <boost/program_options.hpp>
#include <elfio/elfio.hpp>

#include <iostream>
#include <exception>
#include <fstream>
#include <cerrno>
#include <cstdlib> // EXIT_FAILURE
#include <cstring> // std::strerror
#include <string>  // std::char_traits<char>::eof

#include <sys/stat.h> // chmod

/* Both stat and chmod will return 0 on success or -1 on failure and set
 * errno.
 */
int set_executable_permissions(std::string filename)
{
    int rval = 0;
    struct stat st;

    if (stat (filename.c_str(), &st) >= 0)
    {
        /* This will save the user the trouble of running chmod +x on the
         * resulting executable. S_IX* refer to "executable" for "others",
         * "group" and "user" respectively. Refer to the man pages for more
         * information.
         */
        rval = chmod (filename.c_str(), st.st_mode | S_IXOTH | S_IXGRP | S_IXUSR);
    }
    return rval;
}

/* Using the ELFIO library, we will create an ELF executable that
 * consists of the following (equivalent) assembly program:
 *
 * .section text
 *
 *     global _start
 *
 * _start:
 *     ; syscall for exit
 *     mov ebx, 0
 *     mov eax, 1
 *     int 0x80
 *
 * To quote the documentation "3.4 ELF File Writer",
 *
 * "The executable will be created and run on i386 Linux OS platform. It
 * is supposed to run well on both 32 and 64 - bit Linux platforms. The
 * file will be created without invoking the compiler or assembler tools
 * in the usual way (i.e.  translating high level source code that makes
 * use of the standard library functions). Instead, using the ELFIO
 * writer, all the necessary sections and segments of the file will be
 * created and filled explicitly, each, with its appropriate data. The
 * physical file would then be created by the ELFIO library."
 */
void create_executable(std::string filename)
{
    using namespace ELFIO;

    elfio writer;

    /* Initialize empty 'elfio' object. This should be done as the first
     * step when creating a new 'elfio' object as other API is relying
     * on parameters provided – ELF file 32-bits/64-bits and little/big
     * endianness
     */
    writer.create( ELFCLASS64, ELFDATA2LSB );

    /* Other attributes of the file. Linux OS loader does not require
     * full set of the attributes, but they are provided when a regular
     * linker used for creation of ELF files
     */
    writer.set_os_abi( ELFOSABI_LINUX );
    writer.set_type( ET_EXEC );
    writer.set_machine( EM_X86_64 );

    // Create code section
    section* text_sec = writer.sections.add( ".text" );

    /* Set section’s attributes. Section type, flags and alignment have
     * a big significance and controls how this section is treated by a
     * linker or OS loader 
     */
    text_sec->set_type( SHT_PROGBITS );
    text_sec->set_flags( SHF_ALLOC | SHF_EXECINSTR );
    text_sec->set_addr_align( 0x10 );

    // Add data into it
    char text[] = { 
        '\xbb', '\x00', '\x00', '\x00', '\x00',   // mov ebx, 0
        '\xB8', '\x01', '\x00', '\x00', '\x00',   // mov eax, 1           
        '\xCD', '\x80'                            // int 0x80             
    };
    text_sec->set_data( text, sizeof( text ) );

    // Create a loadable segment
    segment* text_seg = writer.segments.add();
    text_seg->set_type( PT_LOAD );
    text_seg->set_virtual_address( 0x08048000 );
    text_seg->set_physical_address( 0x08048000 );
    text_seg->set_flags( PF_X | PF_R );
    text_seg->set_align( 0x1000 );

    // Add code section into program segment
    text_seg->add_section_index( text_sec->get_index(), text_sec->get_addr_align() );

    // Setup entry point
    writer.set_entry( 0x08048000 );

    // Create ELF file
    writer.save( filename );

    if (set_executable_permissions( filename ) == -1)
    {
        std::cerr << "nothing: error: " << std::strerror(errno) << "\n";
        std::cerr << "note: you may have to use chmod +x\n";
    }
}

int main(int argc, char* argv[])
{
    namespace po = boost::program_options;

    try
    {
        bool silent = false;
        if (argc < 2)
        {
            std::cerr << "nothing: fatal error: no input files\n";
            std::cerr << "compilation terminated.\n";
            return EXIT_FAILURE;
        }

        /* The existence of an input-file option may be confusing, so we
         * still allow it to be used but hide it from the help prompt.
         */
        po::options_description visible_options("Allowed options");
        visible_options.add_options()
            ("help", "produce help message")
            ("silent", "suppress error messages")
            ("output-file,o", po::value<std::string>()->default_value("a.out"),
             "output file")
            ;

        po::options_description hidden_options("Hidden options");
        hidden_options.add_options()
            ("input-file", po::value<std::vector<std::string>>(), "input file")
            ;

        po::options_description command_line_options;
        command_line_options.add(visible_options).add(hidden_options);

        po::positional_options_description positional_options;

        // The second parameter specifies the "max count". -1 means
        // unlimited
        positional_options.add("input-file", -1);

        po::variables_map variables_map;
        po::store(po::command_line_parser(argc, argv).
                options(command_line_options).positional(positional_options).run(),
                variables_map);
        po::notify(variables_map);

        if (variables_map.count("help"))
        {
            std::cout << "usage: ./nothing [OPTIONS] <input-file>...\n";
            std::cout << visible_options;
            std::cout.flush();
            return EXIT_SUCCESS;
        }

        if (variables_map.count("silent"))
        {
            silent = true;
        }

        auto input_files = variables_map["input-file"].as<std::vector<std::string>>();

        for (auto&& input_file : input_files)
        {
            std::ifstream program(input_file);

            if (!program)
            {
                if (!silent)
                {
                    /* "errno uses thread-local storage on modern operating systems.
                     * However, there's no guarantee that the fstream functions will
                     * not clobber errno after an errno occurs. The underlying
                     * functions may not set errno at all (direct system calls on
                     * Linux, or Win32). This doesn't work on many real world
                     * implementations."
                     * - strcat,
                     *   <https://stackoverflow.com/questions/17337602/how-to-get-error-message-when-ifstream-open-fails/17338934#comment33958980_17338934>
                     */
                    std::cerr << "nothing: error: " << argv[1] << ": " << std::strerror(errno) << "\n";
                    std::cerr << "nothing: fatal error: no input files\n";
                    std::cerr << "compilation terminated.\n";        
                    return EXIT_FAILURE;
                }
            }

            if (program.peek() != std::char_traits<char>::eof())
            {
                if (!silent)
                {
                    std::cerr << "nothing: fatal error: program is not empty\n";
                    std::cerr << "compilation terminated.\n";
                }
                return EXIT_FAILURE;
            }
        }

        create_executable(variables_map["output-file"].as<std::string>());
    } catch(std::exception& e) {
        std::cerr << "nothing: error: " << e.what() << "\n";
        return EXIT_FAILURE;
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome back! Beautifully crafted follow-up post! What happened to your other accounts? Let me (or any other moderator) know if you would like them merged. \$\endgroup\$ – Mathieu Guindon Sep 3 '15 at 23:34
2
\$\begingroup\$

This seems like a pretty solid solution. I have mainly minor comments.

Comments

The purpose of comments is to explain logic to readers of your code. To that end, try to avoid comments that are trivial. Like:

#include <cstdlib> // EXIT_FAILURE
#include <cstring> // std::strerror
#include <string>  // std::char_traits<char>::eof

#include <sys/stat.h> // chmod

Those comments really achieve nothing for you. Besides, what you need <string> for is std::string, so besides providing no value they're actually misleading.

Similarly, you have this huge block comment explaining about errno using thread-local storage - but you have no threads, so I don't see how that comment is remotely relevant. Just print the error.

Setting Permissions

stat() returns 0 on success, -1 on failure, so checking for >= 0 is weird. Also you're taking the filename by value, which triggers an unnecssary copy. Instead you could do:

int set_executable_permissions(const char* filename)
{
    struct stat st;
    if (stat(filename, &st) == 0) {
        return chmod(filename, st.st_mode | ... );
    }
    else {
        return -1;
    }
}

As-is, if stat() fails, you're returning 0, which seems pretty misleading! Also, people know what chmod is, so you can just do st.st_mode | 0111.

main()

I'd move the whole body into a separate function just so you can remove one layer of indentation:

try {
    return run_compiler(argc, argv);
}
catch (std::exception& e) {
    std::cerr << "nothing: error: " << e.what() << "\n";
    return EXIT_FAILURE;
}

Checking file emptiness

Your check:

program.peek() != std::char_traits<char>::eof()

Preferred:

program.peek() != std::ifstream::traits_type::eof();

Use the traits_type from the stream you're using. It's clearer and doesn't lead to the question of where char_traits comes from.

Also, if the program doesn't exist but silent is false, we still want to fail. So the flow should probably be:

if (!program) {
    if (!silent) {
        std::cerr << ...;
    }
    return EXIT_FAILURE;
}
else if (program.peek() != ... ) {
    if (!silent) {
        std::cerr << ...;
    }
    return EXIT_FAILURE;
}

Furthermore, this sort of calls for a separate function:

bool is_empty_file(std::string const& input_file)
{
    ...
}

that you could just call on each of the files:

if (!std::all_of(input_files.begin(), input_files.end(), is_empty_file))
{
    return EXIT_FAILURE;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.