View Change

1 comment:

• File programmer.h:

  • Patch Set #4, Line 402: #if CONFIG_DUMMY == 1

    Please just drop all these `#if`. The only difference it makes is
    whether we get a compile-time or link-time error if something goes
    wrong. I don't think it matters or even justifies the clutter.
    > Mentioning the file name is also a bit redundant. Maybe just a single
    comment /* programmer drivers */ ?
    > Doesn't have to be in this patch, though

    CB:55387

    Thank you so much! Your long message is very helpful, I think now I actually understand: extern declaration by itself is fine, as long as all the usages are guarded.

    I made CB:55386 on coreboot earlier today. It introduces a helper function to transform a compile-time error into a link-time error, which allows dropping some preprocessor usage in code.

    I just forced build errors by not enabling `DEBUG_BOOT_STATE` and manually inverting the condition in src/lib/hardwaremain.c in order to show you the difference between a compile-time error and a link-time error.

    Compile-time error excerpt:

     src/lib/hardwaremain.c: In function 'bs_call_callbacks':
     src/lib/hardwaremain.c:279:15: error: 'struct boot_state_callback' has no member named 'location'
          bscb, bscb->location);

    The `location` member is inside a text block guarded by a conditional preprocessor directive. Since the `CONFIG(DEBUG_BOOT_STATE)` condition is false, the preprocessor does not pass the contents of this text block to the compiler, and the compiler does not "see" the `location` member in the struct definition. When it reaches the `bscb->location` part, the compiler complains about the non-existent `location` member.

    Link-time error excerpt:

     coreboot-builds/HP_280_G2/generated/ramstage.o: in function `bscb_location':
     src/include/bootstate.h:114: undefined reference to `_dead_code_assertion_failed'

    The `dead_code_t` macro inside the `bscb_location` function evaluates to some expression that calls the `_dead_code_assertion_failed` function, which is intentionally never defined (only declared). Compilation succeeds: the source code can be translated into a `hardwaremain.o` object file, which contains external definitions and references for linking. However, when the linker tries to resolve the external references, it cannot find a definition for the `_dead_code_assertion_failed` function, and bails out.

    The advantage of link-time errors over compile-time errors (i.e. what CB:55386 leverages) is that the compiler can automatically optimise out the code that would cause a link-time error. CB:55386 works because `bscb_location` is never reached when the `CONFIG(DEBUG_BOOT_STATE)` condition is false, and the compiler can thus optimise `bscb_location` away along with the `_dead_code_assertion_failed` function call.

To view, visit change 52946. To unsubscribe, or for help writing mail filters, visit settings.