Yield-WaitFor: Introduce defer and update HMAC as example usage

[ppannuto]

This is a minimal example of what "cleanup attribute" like code would look like.

This probably isn't the exact best way to implement it (due to the "magic variable name" of ret_{param}), but it gives a sense of what it would look like.

[bradjc]

Obviously I'm not a big fan of macros. Seems complicated compared to the goto version.

It also isn't very flexible for drivers that may need more cleanup.

[ppannuto]

Obviously I'm not a big fan of macros. Seems complicated compared to the goto version.

I agree the macro implementation is complex (aren't they all), but I feel like the actual code is simpler and less error-prone. I messed up the incremental conversion when I'd modified just one of the allows doing just this [not that it's hard to see per se, i.e., I deleted the "top" allow call and the "top" goto label when I first tried], but it's sort of emblematic of why this goto approach is error-prone.

It also isn't very flexible for drivers that may need more cleanup.

Yes.. that was to keep the macro simpler. Some of the other flavors of this idea have you explicitly define the "destructor" as part of the invocation of the macro; it's more flexible, but also more complex. I'd imagine if we wanted to go this way keeping the simpler "only-works-for-allow" macro, since "unallow-when-done" is such a common use case, while also providing a more generic option.

[ppannuto]

Okay, I found a much cleaner implementation of this, which matches the (current) draft specification for the next release of C, i.e., in the future defer becomes a keyword, we delete our header, and no code needs to change.

[bradjc]

The latest version is nice, even if I don't understand the C code in the macros. But I'm worried about backwards compatibility.

[ppannuto]

What's the worry w.r.t. backwards compatibility? This defer macro only exists in C files that explicitly include libtock/defer.h, it only changes code where it's used — so you simply write code with defer instead of goto exitN.

As for the macro, the fully expanded form basically does this:

defer { foo(); bar(); }
   // ^^^^^^^^^^^^^^ this bit doesn't change at all

// The above expands to:

auto void __DEFER_FUNCTION0(int* __DEFER_VARIABLE0SENTINEL);  // This is just a function prototype
[[ gnu::cleanup(__DEFER_FUNCTION0) ]] int __DEFER_VARIABLE0; // This declares a variable which is never used, but the compiler can see when it goes out of scope, and will call __DEFER_FUNCTION0 when it goes out of scope
auto void __DEFER_FUNCTION0(__attribute__ ((unused)) int* __DEFER_VARIABLE0SENTINEL) { foo(); bar(); }
                                                                                  // ^^^^^^^^^^^^^^^^^ here's our unchanged code, where it's now the body of a function that's being defined

[ppannuto]

I rebased this on master so the CI can pick up a compiler newer than 2021.

It's now functionally a replacement PR for #532.

[bradjc]

What's the worry w.r.t. backwards compatibility?

I rebased this on master so the CI can pick up a compiler newer than 2021.

I'm worried this won't work for users on Ubuntu 22.04 (a currently supported LTS release).

[ppannuto]

I rebased this on master so the CI can pick up a compiler newer than 2021.

Turns out I didn't need to do that, just needed to do 0b840d9. I added a "legacy-compilers" CI run here that runs on the default LTS compilers for both ARM and RISCV and it seems to work without issue.

(Now, I acknowledge that #539 has some lingering issues that I'm working on, but they may actually be unrelated to compiler version; don't let issues over there poison the well here)

[bradjc]

Oh this does work on jammy?

[ppannuto]

Seems like it, yeah.

[bradjc]

Before this change:

[TEST] HMAC
[0] schedule[0x1:1] @0x41659(0xd, 0x0, 0x0, 0x41601) = Ok(())
[0] function_call @0x41659(0xd, 0x0, 0x0, 0x41601)
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] read-only allow(0x40003, 0, @0x200099d4, 11) = AllowReadOnlySuccess(0x0, 0)
[0] read-only allow(0x40003, 1, @0x200098d4, 72) = AllowReadOnlySuccess(0x0, 0)
[0] read-write allow(0x40003, 2, @0x20009a50, 32) = AllowReadWriteSuccess(0x0, 0)
[0] remove_pending_upcalls[0x40003:0] = 0 upcall(s) removed
[0] subscribe(0x40003, 0, @0x41593, 0x40511) = SubscribeSuccess(0x0, 0)
[0] cmd(0x40003, 1, 0x0, 0x0) = Success
[0] schedule[0x40003:0] @0x41593(0x0, 0xb5, 0x0, 0x40511) = Ok(())
[0] yield. which: 1
[0] function_call @0x41593(0x0, 0xb5, 0x0, 0x40511)
[0] read-only allow(0x40003, 0, @0x0, 0) = AllowReadOnlySuccess(0x200099d4, 11)
[0] read-only allow(0x40003, 1, @0x0, 0) = AllowReadOnlySuccess(0x200098d4, 72)
[0] read-write allow(0x40003, 2, @0x0, 0) = AllowReadWriteSuccess(0x20009a50, 32)
[0] remove_pending_upcalls[0x1:1] = 0 upcall(s) removed
[0] subscribe(0x1, 1, @0x41659, 0x41601) = SubscribeSuccess(0x41659, 267777)
[0] read-only allow(0x1, 1, @0x20009bd0, 89) = AllowReadOnlySuccess(0x20009bd0, 13)
[0] cmd(0x1, 1, 0x59, 0x0) = Success
[0] yield. which: 1
[HMAC] Computed Output: b5055553bd5e75d4fc96bf2559d655b77a58aea8b4c4076c52f774851bba06d8

After this change:

[TEST] HMAC
[0] schedule[0x1:1] @0x415f1(0xd, 0x0, 0x0, 0x41599) = Ok(())
[0] function_call @0x415f1(0xd, 0x0, 0x0, 0x41599)
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] cmd(0x40003, 0, 0x0, 0x0) = Success
[0] read-only allow(0x40003, 0, @0x200099c8, 11) = AllowReadOnlySuccess(0x0, 0)
[0] read-only allow(0x40003, 1, @0x200098c8, 72) = AllowReadOnlySuccess(0x0, 0)
[0] read-write allow(0x40003, 2, @0x20009a44, 32) = AllowReadWriteSuccess(0x0, 0)
[0] cmd(0x40003, 1, 0x0, 0x0) = Success
[0] schedule[0x40003:0] @0x0(0x0, 0xb5, 0x0, 0x0) = Ok(())
[0] yield. which: 2
[0] Yield-WaitFor: [NU] (0x0, 0xb5, 0x0)
[0] read-write allow(0x40003, 2, @0x0, 0) = AllowReadWriteSuccess(0x20009a44, 32)
[0] read-only allow(0x40003, 1, @0x0, 0) = AllowReadOnlySuccess(0x200098c8, 72)
[0] read-only allow(0x40003, 0, @0x0, 0) = AllowReadOnlySuccess(0x200099c8, 11)
[0] remove_pending_upcalls[0x1:1] = 0 upcall(s) removed
[0] subscribe(0x1, 1, @0x415f1, 0x41599) = SubscribeSuccess(0x415f1, 267673)
[0] read-only allow(0x1, 1, @0x20009bc0, 89) = AllowReadOnlySuccess(0x20009bc0, 13)
[0] cmd(0x1, 1, 0x59, 0x0) = Success
[0] yield. which: 1
[HMAC] Computed Output: b5055553bd5e75d4fc96bf2559d655b77a58aea8b4c4076c52f774851bba06d8

So looks good to me. I also forced an error into the libtock-sync hmac library and the unallows worked as expected.

[brghena]

Generally this looks good. I like the defer addition and I do think it makes the error handling cleanup nicer.

I think defer + HMAC stuff could go together. But the CI updates seem separate?

[brghena]

(I must have forgotten to hit the button on this comment) Could we split this out to a separate PR? I think it's separate from the rest of the changes here.

[ppannuto]

Yeah, that doesn't need to be on this PR. I added it to prove that this works with legacy compilers, but it's unrelated. Dropped it here, will add a more CI-focused PR in a sec