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

[dnm] demonstrate that dead code hits assertions #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nvanbenschoten
Copy link

In cockroachdb/apd#103, I'm looking into hooking up gcassert to CI. It's pretty magical to be able to add these assertions in and protect against regressions in carefully tuned code. I'd like to start using it in more places.

Unfortunately, I'm running into the problem that platform-specific conditional logic that leads to compile-time dead code can cause assertion failures if one of these assertions lands in the dead code. I'm not quite sure what to do about that, so I figured I'd open up a PR to demonstrate the problem.

nvanbenschoten added a commit to nvanbenschoten/apd that referenced this pull request Jan 9, 2022
This commit switches the BigInt fast-paths from operating on uint values
to uint64 values. This allows them to work on 32-bit architectures for any
value that can fit in a 64-bit unsigned integer.

There are two benefits of this. The first is that it unifies the arithmetic
fast-paths across the architectures so that they perform more similarly. The
second is that it removes some dead code on 64-bit architectures, so that we
can avoid issues with jordanlewis/gcassert#3.
nvanbenschoten added a commit to nvanbenschoten/apd that referenced this pull request Jan 9, 2022
This commit switches the BigInt fast-paths from operating on uint values
to uint64 values. This allows them to work on 32-bit architectures for any
value that can fit in a 64-bit unsigned integer.

There are two benefits of this. The first is that it unifies the arithmetic
fast-paths across the architectures so that they perform more similarly. The
second is that it removes some dead code on 64-bit architectures, so that we
can avoid issues with jordanlewis/gcassert#3.
nvanbenschoten added a commit to nvanbenschoten/apd that referenced this pull request Jan 9, 2022
This commit switches the BigInt fast-paths from operating on uint values
to uint64 values. This allows them to work on 32-bit architectures for any
value that can fit in a 64-bit unsigned integer.

There are two benefits of this. The first is that it unifies the arithmetic
fast-paths across the architectures so that they perform more similarly. The
second is that it removes some dead code on 64-bit architectures, so that we
can avoid issues with jordanlewis/gcassert#3.
@jordanlewis
Copy link
Owner

I'm not quite sure what to do about this either, unless there's a way to access the dead code analysis pass somehow.

@nvanbenschoten
Copy link
Author

Yeah, I looked into whether there was a way to get debug information from either the branch elimination or (many) dead code analysis passes of gc, but from what I could see, neither export anything.

I found a way to restructure the code to work around this in cockroachdb/apd, so it's no longer blocking that project, but this may come up in other places in the future. One potential workaround is to allow callers to selectively opt-out of function-wide //gcassert:inline directives at specific callsites. Something like a //gcassert:ignore directive.

@jordanlewis
Copy link
Owner

That would work, though wouldn't it have the problem where you wouldn't know which one to ignore if there are 2 should-inline callsites in 2 mutually exclusive compile time if branches?

@nvanbenschoten
Copy link
Author

That does seem like a problem. How does //gcassert:inline work when added to a line with two function calls?

@jordanlewis
Copy link
Owner

I think that it would pass if either of the function calls got inlined. There's just a line number to boolean map for the inline check, it looks like.

@jordanlewis
Copy link
Owner

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.

2 participants