Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not check string size for printf()'s format string #1538

Closed
wants to merge 4 commits into from
Closed

Do not check string size for printf()'s format string #1538

wants to merge 4 commits into from

Conversation

yzhao1012
Copy link
Contributor

@yzhao1012 yzhao1012 commented Sep 26, 2020

And create String with its original size, without truncation.

This allows having longer format string in printf() statement,
and does not affect existing functionality.

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

And create String with its original size, without truncation.
@yzhao1012
Copy link
Contributor Author

Not a language change.

Need help with where to add the tests.

@@ -76,6 +76,14 @@ void SemanticAnalyser::visit(PositionalParameter &param)

void SemanticAnalyser::visit(String &string)
{
// Skip check for printf()'s format string (1st argument) and create the string
// with the original size. This is because format string is not part of bpf byte code.
if (func_ == "printf" && func_arg_idx_ == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a shame we cannot use is_compiel_time_func here. I've created #1539 to make that possible.

@fbs
Copy link
Contributor

fbs commented Sep 26, 2020

Thanks!

Need help with where to add the tests.

You generally want to add a test for the pass you modified. So in this case in tests/semantic_analyser.cpp you can extend TEST(semantic_analyser, printf) with a case for this. And please add a runtime test too.

Could you also run clang format and squash all the commits together?

src/ast/semantic_analyser.h Outdated Show resolved Hide resolved
@yzhao1012
Copy link
Contributor Author

Thanks!

Need help with where to add the tests.

You generally want to add a test for the pass you modified. So in this case in tests/semantic_analyser.cpp you can extend TEST(semantic_analyser, printf) with a case for this. And please add a runtime test too.

Could you also run clang format and squash all the commits together?

Fixed clang-format issue.

Working on test now.

@fbs
Copy link
Contributor

fbs commented Oct 20, 2020

@yzhao1012, can I help you with the tests? This would be good to get this in :)

@yzhao1012
Copy link
Contributor Author

@yzhao1012, can I help you with the tests? This would be good to get this in :)

It would be great if you can shoulder the task of writing tests.

I am not sure what form of collaboration is the best here. It's totally OK to me if you can just copy or rewrite this PR in a new one that you create from scratch.

Or you were thinking to give specific instructions on writing tests?

@fbs
Copy link
Contributor

fbs commented Nov 2, 2020

I was thinking simple something like this

TEST(semantic_analyser, call_printf)
{
  std::stringstream prog;

  prog << "i:ms:100 { printf(\"" << std::string("a", 200) << " %d\\n\", 1); }"
  test(prog, 0);
}

@yzhao1012
Copy link
Contributor Author

Thanks!

Need help with where to add the tests.

You generally want to add a test for the pass you modified. So in this case in tests/semantic_analyser.cpp you can extend TEST(semantic_analyser, printf) with a case for this. And please add a runtime test too.
Could you also run clang format and squash all the commits together?

Fixed clang-format issue.

Working on test now.

How could I run semantic_analyser.cpp?

I was reading https://github.com/iovisor/bpftrace/tree/master/tests but could not find the exact instructions on how to run an individual test.

@fbs
Copy link
Contributor

fbs commented Nov 2, 2020

if you want an individual test you can do ./tests/bpftrace_test --gtest_filter="semantic_analyser.call_printf" but you can just run the whole testsuite.

@fbs
Copy link
Contributor

fbs commented Nov 23, 2020

@yzhao1012 if you don't mind I'll steal this, rebase and add the tests and then get it merged :). Its nice to have

@fbs fbs mentioned this pull request Nov 23, 2020
@fbs fbs closed this Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants