m4b/faerie

API details

sunfishcode opened this issue · 7 comments

If I declare a symbol as a DataImport and then later declare it as a Data, like this:

    let mut obj = Artifact::new(Target::X86_64, Some("t.o".into()));

    obj.declare("str.0", faerie::Decl::DataImport {}).expect(
        "declare",
    );

    obj.declare(
        &"str.0".to_string(),
        faerie::Decl::Data {
            global: false,
            writeable: false,
        },
    ).expect("declare");

    obj.define("str.0", "hello world\0".to_string().into_bytes())
        .expect("define");

I get an error:

thread 'main' panicked at 'declare: DuplicateDeclaration("str.0")', /checkout/src/libcore/result.rs:916:4

If I reverse the order of the Data and DataImport declarations, it looks like the DataImport silently wins; I get:

thread 'main' panicked at 'define: ImportDefined("str.0")', /checkout/src/libcore/result.rs:916:4

I kind of expect faerie to accept this, with a Data declaration overriding a DataImport, regardless of which comes first.

Possibly the source of my expectation is C, where code like this:

extern int x;
int x = 2;
extern int x;

compiles and the definition "wins". But also, I would find it convenient in some situations, because while I usually know up front the set of things that will be defined, I don't always know up front what's going to be imported unless I do some extra work.

m4b commented

Wouldn't a more accurate C version of the above be something:

extern int x;
int x = 2;
extern float x;

int main() {
  return 0;
}

which does not compile with either clang or gcc (as expected):

test.c:3:14: error: conflicting types for ‘x’
 extern float x;
              ^
test.c:2:5: note: previous definition of ‘x’ was here
 int x = 2;
     ^
m4b@efrit ::  [ /tmp ] clang test.c
test.c:3:14: error: redeclaration of 'x' with a different type: 'float' vs 'int'
extern float x;
             ^
test.c:2:5: note: previous definition is here
int x = 2;
    ^
1 error generated.

We can easily ignore duplicate exact declarations; but imho declaring the same symbol with different types and semantics, just seems like a fundamental compiler error, no?

m4b commented

For cases like:

extern int x;
int x = 2;

int main() {
  return 0;
}

which would be an import decl and then a definition, we could relax this for imports and allow the definition to take place, when the types exactly match (and subsequent import calls are no-ops).

But anything else just seems like the wild west to me, but I dunno :P

m4b commented

Ah i see now you had Data, for some reason reading it on my phone I thought it was a data and then a cstr declaration; yes I agree, behavior when the declaration matches the definition should be C-ish, basically:

  1. Duplicate declarations of same type are no-ops
  2. Declaration, definition must match as usual, OR decl is an import and same datatype
  3. definition, declaration, when types match OR its an import decl are no ops.

I think these are the rules you want, yes?

Yes, that sounds right.

m4b commented

98cb29f caused a regression :/

  11: faerie::elf::Elf::link
             at src/elf.rs:464
  12: <faerie::elf::Elf<'a> as faerie::artifact::Object>::to_bytes
             at src/elf.rs:671
  13: faerie::artifact::Artifact::emit
             at ./src/artifact.rs:384
  14: faerie::artifact::Artifact::write
             at ./src/artifact.rs:388
  15: prototype::run
             at src/bin/main.rs:128
  16: prototype::main
             at src/bin/main.rs:178
  17: __rust_maybe_catch_panic
             at /checkout/src/libpanic_unwind/lib.rs:101
  18: std::rt::lang_start
             at /checkout/src/libstd/panicking.rs:459
             at /checkout/src/libstd/rt.rs:58
  19: main
  20: __libc_start_main
  21: _start
m4b commented

heh; i have absolutely no idea why/how this could have caused a regression at this point ;)

m4b commented

So the problem was I deleted the import updating logic 😭

A lot of this will be caught now, as the CI runs the tests, runs the prototype to build a deadbeef.o, then runs it again on the code similar to whats in readme, linked with deadbeef.o, and then invokes the linker on the result, and then runs the resulting binary.