Add support for strftime() builtin #1387

suyuee · 2020-06-16T18:56:25Z

This PR introduces a new builtin function strftime. This function makes it possible to print the current time with printf:

bpftrace -e 'i:s:1 { printf("%s\n", strftime("%H:%M:%S", nsecs)); }'

Although time can print current time on its own, printing timestamp with printf can produce more concise and more readable output. Resolves #1337 .

Checklist

Language changes are updated in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md

src/ast/semantic_analyser.cpp

tests/semantic_analyser.cpp

brendangregg · 2020-06-17T05:48:56Z

What is this?

TEST(semantic_analyser, call_timestamp_warnings)
{
  test_for_warning("kprobe:f { time(); }", "deprecated");
  test_for_warning("kprobe:f { time(\"%M:%S\"); }", "deprecated");
  test_for_warning("kprobe:f { time(\"%M:%S\", nsecs); }", "deprecated", true);
}

Does this deprecate/break the old behavior? I don't see a compelling reason why we should. It's in ~10k printed copies of the BPF book that we cannot update, so no.

I also saw this (there's no reference docs, so it's not clear what you're doing):

          warning("Using time() to implicitly print time is deprecated. Print "
                  "the returned value instead.",

I called it time() because it was not strftime(). This sounds like you're turning it into strftime(). If you want strftime(), add a new thing called strftime() and leave time() alone.

brendangregg · 2020-06-22T08:15:08Z

Why create a new printf() format character? Why not "%s"?

See printf.cpp: we convert a lot of types to String when printed.

    if (arg_type == Type::ksym || arg_type == Type::usym || arg_type == Type::probe ||
        arg_type == Type::username || arg_type == Type::kstack || arg_type == Type::ustack ||
        arg_type == Type::inet)
      arg_type = Type::string; // Symbols should be printed as strings

suyuee · 2020-06-22T19:38:22Z

@brendangregg The reason we hesitated to use "%s" was that we were afraid it would send a message to user that strftime() returns a string and might cause some confusion. It's tempting to link "strftime()" with "string()". For example, people may end up using 'strncmp()` on the return value, which doesn't make sense.

Using "%ts" doesn't have that issue. But the problem with it is that creating a new specifier for the old friend printf() is not actually user-friendly either. It's hard to remember and lacks simplicity.

I think the async nature of strftime() makes it hard to make perfect decisions for users, since people might not always be aware that BPF programs come with many limitations.

brendangregg · 2020-06-22T21:13:46Z

A) If it's called strftime() then people will use it with %s. (Where did %ts come from anyway? POSIX? I'm not familiar with it.)

B) I don't really see the use case other than with printf().

C) That ship has sailed anyway: we have things like kstack() and username() that use %s, but you can't use with strcmp().

It's a tracing language, not an application language. It only needs to support enough for common case tools -- anything more complex can be deferred to BCC. The existence of BCC allows us to keep bpftrace simple.

src/types.cpp

src/types.h

docs/reference_guide.md

src/ast/async_event_types.h

src/ast/codegen_llvm.cpp

docs/reference_guide.md

src/bpftrace.cpp

suyuee · 2020-07-07T16:32:55Z

Ready for review.

fbs

Some small things but looks good overal :)

fbs · 2020-07-07T23:43:03Z

src/ast/irbuilderbpf.cpp

@@ -273,7 +273,7 @@ Value *IRBuilderBPF::CreateMapLookupElem(Value *ctx,
  CreateCondBr(condition, lookup_success_block, lookup_failure_block);

  SetInsertPoint(lookup_success_block);
-  if (type.IsAggregate())
+  if (type.IsAggregate() || type.IsTimestampTy())


I think we should either put timestamp in IsAggregate or find a new name to describe them both (and all future async events which should be handled like this).

I'd agree. I think IsCompound could be a nice name here since C++ seems to use similar term: std::is_compound and it includes array, tuple, class and so on (Type::timestamp seems like a class type to me). Btw there could be some other options like IsCollectionTy, IsComposite and IsCombined though I'm not sure if there are as good.

Discussed with @danobi and thought that it might be good to add a helper function instead.

fbs · 2020-07-07T23:45:55Z

src/bpftrace.cpp

@@ -1673,6 +1686,52 @@ std::string BPFtrace::resolve_uid(uintptr_t addr) const
  return username;
 }

+std::string BPFtrace::resolve_timestamp(uint32_t strftime_id,


Looks like this currently gives us 1s accuracy right? I was hoping for to get closer to 100us but that should probably go in a follow up ticket.

Yes. If we want someting better, we may have to choose something more accurate than btime. clock_gettime(CLOCK_REALTIME) might help though I'm not so sure if that's what we want; and if we use it, it wouldn't make sense to use nsecs as the second argument of strftime since it means 'nsecs since boot time'.

src/bpftrace.cpp

src/ast/async_event_types.h

docs/reference_guide.md

src/ast/codegen_llvm.cpp

docs/reference_guide.md

CHANGELOG.md

This introduces a new builtin function `strftime`. This function makes it possible to print the current time with `printf`: ``` bpftrace -e 'i:s:1 { printf("%s\n", strftime("%H:%M:%S", nsecs)); }' ``` Although `time` can print current time on its own, printing timestamp with `printf` can produce more concise and more readable output. Resolves bpftrace#1337 .

suyuee changed the title ~~Add semantic analysis support for 2-arg time() builtin~~ Add support for 2-arg time() builtin Jun 16, 2020

danobi reviewed Jun 16, 2020

View reviewed changes

suyuee force-pushed the enhance_time_call branch from 473f268 to cd8b28f Compare June 17, 2020 00:37

suyuee force-pushed the enhance_time_call branch from cd8b28f to 5d7c690 Compare June 17, 2020 17:28

suyuee changed the title ~~Add support for 2-arg time() builtin~~ Add support for strftime() builtin Jun 17, 2020

suyuee changed the title ~~Add support for strftime() builtin~~ Add support for strftime() builtin Jun 17, 2020

suyuee changed the title ~~Add support for strftime() builtin~~ Add support for strftime() builtin Jun 17, 2020

suyuee force-pushed the enhance_time_call branch from 5d7c690 to ed4a421 Compare June 17, 2020 21:20

suyuee force-pushed the enhance_time_call branch 3 times, most recently from 86fb9ff to ea2bb2d Compare June 29, 2020 23:06

suyuee marked this pull request as ready for review June 30, 2020 01:17

fbs added this to the 0.11.0 milestone Jul 2, 2020

danobi requested changes Jul 2, 2020

View reviewed changes

danobi mentioned this pull request Jul 2, 2020

can i get millisecond in time() format? #1406

Closed

suyuee marked this pull request as draft July 3, 2020 05:04

suyuee force-pushed the enhance_time_call branch 5 times, most recently from 1bb53e9 to c29983b Compare July 7, 2020 16:05

suyuee marked this pull request as ready for review July 7, 2020 16:32

fbs requested changes Jul 7, 2020

View reviewed changes

suyuee force-pushed the enhance_time_call branch 2 times, most recently from e9f83b2 to 84adc3d Compare July 8, 2020 23:51

suyuee force-pushed the enhance_time_call branch 2 times, most recently from 8d2b3d1 to 5800d4b Compare July 9, 2020 00:15

fbs requested changes Jul 11, 2020

View reviewed changes

docs/reference_guide.md Outdated Show resolved Hide resolved

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

docs/reference_guide.md Show resolved Hide resolved

suyuee force-pushed the enhance_time_call branch from 5800d4b to 5bb7d3e Compare July 13, 2020 18:50

fbs modified the milestones: 0.11.0, 0.12.0 Jul 15, 2020

fbs reviewed Jul 15, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

suyuee added 3 commits July 15, 2020 14:49

Add semantic analysis support for strftime() builtin

d135dfb

Add CodeGen for strftime() builtin

0b06c80

suyuee force-pushed the enhance_time_call branch from 5bb7d3e to 5fff47e Compare July 15, 2020 20:13

fbs approved these changes Jul 15, 2020

View reviewed changes

danobi approved these changes Jul 16, 2020

View reviewed changes

fbs merged commit 5bb653c into bpftrace:master Jul 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for strftime() builtin #1387

Add support for strftime() builtin #1387

suyuee commented Jun 16, 2020 •

edited

Loading

brendangregg commented Jun 17, 2020

brendangregg commented Jun 22, 2020

suyuee commented Jun 22, 2020 •

edited

Loading

brendangregg commented Jun 22, 2020

suyuee commented Jul 7, 2020

fbs left a comment

fbs Jul 7, 2020

suyuee Jul 8, 2020

suyuee Jul 8, 2020

fbs Jul 7, 2020

suyuee Jul 8, 2020 •

edited

Loading

Add support for strftime() builtin #1387

Add support for strftime() builtin #1387

Conversation

suyuee commented Jun 16, 2020 • edited Loading

Checklist

brendangregg commented Jun 17, 2020

brendangregg commented Jun 22, 2020

suyuee commented Jun 22, 2020 • edited Loading

brendangregg commented Jun 22, 2020

suyuee commented Jul 7, 2020

fbs left a comment

Choose a reason for hiding this comment

fbs Jul 7, 2020

Choose a reason for hiding this comment

suyuee Jul 8, 2020

Choose a reason for hiding this comment

suyuee Jul 8, 2020

Choose a reason for hiding this comment

fbs Jul 7, 2020

Choose a reason for hiding this comment

suyuee Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

suyuee commented Jun 16, 2020 •

edited

Loading

suyuee commented Jun 22, 2020 •

edited

Loading

suyuee Jul 8, 2020 •

edited

Loading