Skip to content

Add support for literal zero in FnPtrNull rewriter pass #438

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.

Sign up for GitHub

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

Open
ayrtonm wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add support for literal zero in FnPtrNull rewriter pass #438

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c538d8f
tests: Test for rewriting literal zeros used as function pointers
ayrtonm Oct 9, 2024
8c7e95f
rewriter: Match literal zeros used as function pointers in FnPtrNull
ayrtonm Oct 9, 2024
8af390e
tests: Update tests where rewriter input doesn't depend on NULL
ayrtonm Oct 9, 2024
5c60763
rewriter: Make FnPtrNull output not depend on NULL if input doesn't
ayrtonm Oct 9, 2024
ce03676
wip
ayrtonm Oct 9, 2024
dfa579a
wip tests
ayrtonm Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions tests/rewrite_fn_ptr_eq/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ Test(rewrite_fn_ptr_eq, main) {
int res;
int *y = &res;
void *x = NULL;
// REWRITER: bin_op fn = IA2_FN(add);
bin_op fn = add;
// REWRITER: bin_op fn2 = { NULL };
bin_op fn2 = NULL;

// Check that pointers for types other than functions are not rewritten
Expand Down Expand Up @@ -78,4 +80,31 @@ Test(rewrite_fn_ptr_eq, main) {
if (y || !fn) { }
// REWRITER: if (x && IA2_ADDR(fn) && y) { }
if (x && fn && y) { }

// REWRITER: fn = (typeof(fn)) { NULL };
fn = NULL;
Comment on lines +84 to +85
Copy link
Contributor

@randomPoison randomPoison Oct 9, 2024

Choose a reason for hiding this comment

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

I don't think this case is handled correctly if using a define other than NULL. In zlib they use a custom ZNULL define (still just 0), and when running the rewriter on this branch it seems like ZNULL gets replaced by 0. For example:

strm->zalloc = Z_NULL;

gets rewritten to

strm->zfree = (typeof(strm->zfree)) { 0 };

I'm not sure if that's something we can or care to fix, since it doesn't really have a functional difference.

Copy link
Contributor Author

@ayrtonm ayrtonm Oct 10, 2024

Choose a reason for hiding this comment

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

The output is totatlly broken rn but that's a good catch about custom NULL macros. What I had in mind would've generated (typeof(f)) { NULL } which wouldn't be the best thing to do. None of this is developer-facing though so I might just have all cases use 0 instead of the corresponding macros.


// the following tests don't use NULL so the rewriter output shouldn't rely on it either
#undef NULL
// REWRITER: bin_op fn3 = { 0 };
bin_op fn3 = 0;

// REWRITER: fn = (typeof(fn)) { 0 };
fn = 0;

// check that literal zeroes aren't rewritten if not cast to function pointers
// REWRITER: res = 0;
res = 0;

// REWRITER: if (IA2_ADDR(fn) == 0) { }
if (fn == 0) { }

// REWRITER: if (IA2_ADDR(mod.fn) == 0) { }
if (mod.fn == 0) { }

// REWRITER: if (IA2_ADDR(fn) == 0) { }
if (fn == (typeof(fn))0) { }

// REWRITER: if (IA2_ADDR(mod.fn) == 0) { }
if (mod.fn == (typeof(fn))0) { }
}
10 changes: 8 additions & 2 deletions tools/rewriter/SourceRewriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,9 @@ class FnPtrNull : public RefactoringCallback {

auto fn_ptr_typedef = hasType(typedefNameDecl(hasType(fn_ptr)));

auto null_expr = implicitCastExpr(ignoringParenCasts(nullPointerConstant()))
auto zero_literal = integerLiteral(equals(0)).bind("literalZero");
auto null_macro = nullPointerConstant();
auto null_expr = implicitCastExpr(ignoringParenCasts(anyOf(null_macro, zero_literal)))
.bind("nullExpr");

auto null_fn_ptr = varDecl(hasInitializer(null_expr),
Expand All @@ -418,6 +420,7 @@ class FnPtrNull : public RefactoringCallback {
refactorer.addMatcher(assign_null, this);
}
virtual void run(const MatchFinder::MatchResult &result) {
bool spelled_as_zero = result.Nodes.getNodeAs<clang::IntegerLiteral>("literalZero");
// The two matchers both have a nullExpr node so this getNodeAs can't fail
auto *null_fn_ptr = result.Nodes.getNodeAs<clang::Expr>("nullExpr");
assert(null_fn_ptr != nullptr);
Expand All @@ -436,14 +439,17 @@ class FnPtrNull : public RefactoringCallback {
Filename filename = get_expansion_filename(loc, sm);

std::string new_expr = "{ NULL }";
if (spelled_as_zero) {
new_expr = "{ 0 }";
}
// If the matcher found an assignment add the type of the LHS variable to
// new_expr
if (auto *lhs_ptr = result.Nodes.getNodeAs<clang::Expr>("ptrLHS")) {
auto char_range =
clang::CharSourceRange::getTokenRange(lhs_ptr->getSourceRange());
auto lhs_binding =
clang::Lexer::getSourceText(char_range, sm, ctxt.getLangOpts());
new_expr = "(typeof("s + lhs_binding.str() + ")) { NULL }";
new_expr = "(typeof("s + lhs_binding.str() + ")) " + new_expr;
}

clang::CharSourceRange expansion_range = sm.getExpansionRange(loc);
Expand Down