Add built-in function for formatting MAC addresses #1647

acj · 2020-11-22T23:21:50Z

This adds a helper function named macaddr (tentative name) that formats MAC addresses in the canonical hex style, e.g. 00:1C:42:B5:20:09. Resolves #1575.

WIP, but feedback is welcome and appreciated.

Checklist

Language changes are updated in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

acj · 2020-11-22T23:25:04Z

The codegen validator is currently failing with an error that I don't understand:

4: /usr/bin/llvm-as-7: /home/runner/work/bpftrace/bpftrace/tests/codegen/llvm/call_macaddr.ll:24:9: error: stored value and pointer type do not match
4: store i64* %macaddr, [6 x i8]* %"@x_val"
4:         ^

Fiddling with the codegen types, I can even get this:

4: /usr/bin/llvm-as-7: /vagrant/tests/codegen/llvm/call_macaddr.ll:24:9: error: stored value and pointer type do not match
4:   store [8 x i8]* %macaddr, [8 x i8]* %"@x_val"
4:         ^

... which seems odd. If I remove the @ = assignment from the codegen test, then everything passes, but I think it should be possible for the current test to pass. Any tips?

fbs

Sweet! Some comments.

Don't really like the name, don't really know anything better either.

src/ast/codegen_llvm.cpp

src/bpftrace.cpp

tests/semantic_analyser.cpp

fbs

Looks good but I think it will have some issues with less common types

Changelog and refguide entries need to be added at some point too

src/ast/semantic_analyser.cpp

fbs · 2020-11-23T10:42:45Z

src/ast/codegen_llvm.cpp

+    auto macaddr = call.vargs->front();
+    auto scoped_del = accept(macaddr);
+
+    b_.CreateProbeRead(ctx_,


IIRC arrays can end up on stack already so this would probe read from the stack in that case

How should I check for that case? I see that shouldBeOnStackAlready doesn't include arrays, so it wouldn't be enough to branch on that. But I'm thinking that we need to branch on something and then store (instead of probe read) if the arg is already on the stack.

Yes it would have to load/memcpy instead of probe reading.

I dont know the best way to check, arrays need some love. I'd be ok with this probe reading for now too, can always fix it in the future.

src/ast/codegen_llvm.cpp

fbs · 2020-11-23T10:44:16Z

tests/semantic_analyser.cpp

+{
+  std::string structs = "struct mac { char addr[6]; }; ";
+
+  test(structs + "kprobe:f { macaddr((struct mac*)arg0); }", 0);


need some tests (and runtime tests) for the other supported cases too, like

macaddr(arg0)
macaddr((struct X)y)
macaddr(somekindofarray)

I'm not sure what you meant for these two:

macaddr((struct X)y)

How would it differ from this line? Do you mean a cast to a non-pointer struct type?

macaddr(somekindofarray)

The line below this one is testing an array, no?

oh woops, codeblock makes the difference

macaddr(arg0) macaddr(*(struct mac*)y) macaddr(somekindofarray)

macaddr(somekindofarray)

The line below this one is testing an array, no?

I'm not sure, I just want to make sure it works for data that has already been loaded onto the stack (so no probe_read required).

acj · 2020-12-14T00:48:29Z

@fbs ping

fbs · 2020-12-14T14:05:57Z

Sorry, missed this one, busy times :(

acj · 2020-12-14T14:17:59Z

All good! I should have time this week to finish testing the different cases, update the docs, etc.

acj · 2020-12-22T03:38:02Z

@fbs ready for another look when you have a moment

fbs · 2021-01-24T10:28:56Z

@acj sorry for all the delays. Can you rebase this so we can get it in?

acj · 2021-01-24T19:57:10Z

@fbs All good. Here's the diff addressing the type changes and fixing a logging typo:

--- src/ast/semantic_analyser.cpp
+++ src/ast/semantic_analyser.cpp
@@ -1194,14 +1194,14 @@ void SemanticAnalyser::visit(Call &call)

     auto &arg = call.vargs->at(0);

-    if (!arg->type.IsIntTy() && !arg->type.IsArray() && !arg->type.IsPtrTy())
+    if (!arg->type.IsIntTy() && !arg->type.IsArrayTy() &&
+        !arg->type.IsByteArray() && !arg->type.IsPtrTy() &&
+        !arg->type.IsRecordTy())
       LOG(ERROR, call.loc, err_)
-          << "() only supports array or pointer arguments"
+          << call.func << "() only supports array or pointer arguments"
           << " (" << arg->type.type << " provided)";

     auto type = arg->type;
-
-    if (arg->type.IsArray() && type.GetSize() != 6)
+    if ((type.IsArrayTy() || type.IsByteArray()) && type.GetSize() != 6)
       LOG(ERROR, call.loc, err_)
           << call.func << "() argument must be 6 bytes in size";

fbs · 2021-01-30T20:34:23Z

src/ast/semantic_analyser.cpp

+
+    if (!arg->type.IsIntTy() && !arg->type.IsArrayTy() &&
+        !arg->type.IsByteArray() && !arg->type.IsPtrTy() &&
+        !arg->type.IsRecordTy())


type::record should only be structs not struct pointers so this should be removed right?

Yep, good catch. Fixed in latest commit

fbs requested changes Nov 22, 2020

View reviewed changes

src/ast/codegen_llvm.cpp Outdated Show resolved Hide resolved

src/bpftrace.cpp Outdated Show resolved Hide resolved

tests/semantic_analyser.cpp Show resolved Hide resolved

acj force-pushed the mac-address-printer branch 2 times, most recently from 4060588 to dcd57db Compare November 23, 2020 00:28

fbs reviewed Nov 23, 2020

View reviewed changes

acj force-pushed the mac-address-printer branch 3 times, most recently from e7120f1 to 87f1ea2 Compare December 22, 2020 02:48

acj marked this pull request as ready for review December 22, 2020 03:37

acj force-pushed the mac-address-printer branch from 87f1ea2 to 26c2d9c Compare January 6, 2021 01:19

fbs approved these changes Jan 14, 2021

View reviewed changes

acj force-pushed the mac-address-printer branch from 26c2d9c to 56ba7bd Compare January 24, 2021 19:45

fbs reviewed Jan 30, 2021

View reviewed changes

Add macaddr built-in function/helper for formatting MAC addresses

b1e7f73

acj force-pushed the mac-address-printer branch from 56ba7bd to b1e7f73 Compare January 31, 2021 01:10

fbs merged commit 04dd8d4 into bpftrace:master Jan 31, 2021

acj deleted the mac-address-printer branch January 31, 2021 12:58

viktormalik mentioned this pull request Feb 9, 2021

macaddr runtime tests failing #1687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add built-in function for formatting MAC addresses #1647

Add built-in function for formatting MAC addresses #1647

acj commented Nov 22, 2020 •

edited

Loading

acj commented Nov 22, 2020

fbs left a comment

fbs left a comment

fbs Nov 23, 2020

acj Nov 25, 2020

fbs Dec 14, 2020

fbs Nov 23, 2020

acj Nov 25, 2020

fbs Dec 14, 2020

acj commented Dec 14, 2020

fbs commented Dec 14, 2020

acj commented Dec 14, 2020

acj commented Dec 22, 2020

fbs commented Jan 24, 2021

acj commented Jan 24, 2021

fbs Jan 30, 2021

acj Jan 31, 2021

Add built-in function for formatting MAC addresses #1647

Add built-in function for formatting MAC addresses #1647

Conversation

acj commented Nov 22, 2020 • edited Loading

Checklist

acj commented Nov 22, 2020

fbs left a comment

Choose a reason for hiding this comment

fbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acj commented Dec 14, 2020

fbs commented Dec 14, 2020

acj commented Dec 14, 2020

acj commented Dec 22, 2020

fbs commented Jan 24, 2021

acj commented Jan 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acj commented Nov 22, 2020 •

edited

Loading