Skip to content

Conversation

nvb
Copy link

@nvb nvb commented Jan 6, 2022

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.

nvb added a commit to nvb/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.
nvb added a commit to nvb/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.
nvb added a commit to nvb/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.

@nvb
Copy link
Author

nvb commented Jan 10, 2022

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?

@nvb
Copy link
Author

nvb commented Jan 10, 2022

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