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

Consider a higher-level representation for checked memory functions #1051

Open
smeenai opened this issue Nov 4, 2024 · 2 comments
Open

Consider a higher-level representation for checked memory functions #1051

smeenai opened this issue Nov 4, 2024 · 2 comments
Labels
IR design Considerations around the design of ClangIR

Comments

@smeenai
Copy link
Collaborator

smeenai commented Nov 4, 2024

__memcpy_chk and __memset_chk are libc functions to support FORTIFY (the bionic docs are a good introduction for the curious). In addition to a count parameter describing how many bytes to copy, they also take a size parameter for the size of the destination buffer, and the function fails at runtime if the count is greater than the size.

Clang implements builtins which call the unchecked functions if the count is known to be <= the size at compile time and call the checked functions otherwise. See https://godbolt.org/z/89dzxadjo for some examples. In #1032, ClangIR implements __builtin___memcpy_chk using the same strategy (and __builtin___memset_chk will follow shortly).

We can potentially instead extend cir.libc.mem* to take in additional information for the checked variants, e.g.

cir.libc.memcpy checked %1 bytes from %2 to %3 of size %4 bytes

This would potentially allow us to constant-fold more cases after CIR optimizations. LLVM has a TargetLibraryInfo hook for __memcpy_chk and can fold some cases that Clang misses, but it also doesn't catch all the cases it could (as seen in the above Godbolt link, where __memcpy_chk(dst, src, n, n) gets simplified but __memcpy_chk(dst, src, n, n + 1) doesn't). It should be pretty doable to fix that though, and I don't know if we'll gain much from an optimization standpoint with the higher-level ClangIR optimization, but it's a nicer design and keeps more information around, which can always be useful.

@smeenai smeenai added the IR design Considerations around the design of ClangIR label Nov 4, 2024
@smeenai
Copy link
Collaborator Author

smeenai commented Nov 4, 2024

One other nice thing about having an operation representation for __memcpy_chk would be that we could implement the transform as an operation fold instead of ad-hoc logic. We'd have to figure out how to make the Expr::EvaluateAsInt machinery available to the fold though.

@bcardosolopes
Copy link
Member

We'd have to figure out how to make the Expr::EvaluateAsInt machinery available to the fold though

If it's simple we can just walk the chain, another solution would be leverage SCCP.

smeenai added a commit to smeenai/clangir that referenced this issue Nov 4, 2024
This follows the same implementation as CodeGen. llvm#1051
tracks potentially switching to a different strategy in the future.
smeenai added a commit that referenced this issue Nov 4, 2024
This follows the same implementation as CodeGen. #1051
tracks potentially switching to a different strategy in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IR design Considerations around the design of ClangIR
Projects
None yet
Development

No branches or pull requests

2 participants