-
Notifications
You must be signed in to change notification settings - Fork 254
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
Check for C11 atomics in and around VisitQualType #302
base: master
Are you sure you want to change the base?
Check for C11 atomics in and around VisitQualType #302
Conversation
As with #291, builds pass locally: $ ./scripts/test_translator.py ./tests
[...]
Test summary:
unexpected failures: 0
unexpected successes: 0
expected failures: 3
successes: 108 |
@@ -157,6 +157,24 @@ class TypeEncoder final : public TypeVisitor<TypeEncoder> { | |||
astEncoder(ast) {} | |||
|
|||
void VisitQualType(const QualType &QT) { | |||
if(isa<AtomicType>(QT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks like this could be QT.isAtomicType()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope -- gives error: ‘const class clang::QualType’ has no member named ‘isAtomicType’
. (When applied only here, and also for other locations which was what I first tried as TBH the isa<AtomicType>
is something I plainly copy-pasted from other locations in this C++ file).
@@ -2104,6 +2122,17 @@ class TranslateASTVisitor final | |||
cbor_encode_boolean(array, D->isImplicit()); | |||
}); | |||
|
|||
if(isa<AtomicType>(typeForDecl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comment.
@chrysn thanks for this PR. I fixed the CI so if you push the minor changes I recommend, we should be able to get this tested and merged in. |
04d23a7
to
44f4cd2
Compare
Force-pushed with new time stamp to trigger CI rebuild. |
Hm, stuck in CI again. Should I try to get it unstuck where it is, or just rebase onto master and see how things go there? |
If you have time to poke at this again, it would be nice to rebase it onto master and make sure everything is green. I've seen our CI being randomly flaky before. |
This does not help towards the support of C11 atomics, but ensures that the existing error messages about the trouble with them get shown: * In VisitQualType at latest, where the source location information is already gone so it's not that helpful (but at least it should catch a wide range of sources where those types could come from) * In VisitTypedefNameDecl, where the `typedef _Atomic(...) atomic_...;` declarations of clang's stdatomic.h run through. It's but one of many possible callers, but it's the one where one more check can make the difference between a usable and a cryptic error for many cases. Closes: immunant#293
44f4cd2
to
886c219
Compare
Rebased, and checked that the copy-pasted idioms are still around the same way at their source location. After I've discovered that I've always been running with outdated builds ( The original testing procedure is still accurate. If you want to trigger the "No precise location information" case, try C like void x(_Atomic(char) *c) { } I'm sure there are places in the code where this could be caught with location information, but I'd say that constructs like the above are rare anyway, and that it's good to have a fallback in case we missed any other location. |
@@ -189,6 +189,24 @@ class TypeEncoder final : public TypeVisitor<TypeEncoder> { | |||
astEncoder(ast) {} | |||
|
|||
void VisitQualType(const QualType &QT) { | |||
if(isa<AtomicType>(QT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check needs to be moved inside the if (!QT.isNull()) {
conditional below; otherwise we get an internal Clang abort doing isa<AtomicType>(QT)
when QT has a null inner pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented inline about issue causing test failures.
By the way, the issue with |
This does not help towards the support of C11 atomics, but ensures that
the existing error messages about the trouble with them get shown:
In VisitQualType at latest, where the source location information is
already gone so it's not that helpful (but at least it should catch a
wide range of sources where those types could come from)
In VisitTypedefNameDecl, where the
typedef _Atomic(...) atomic_...;
declarations of clang's stdatomic.h run through. It's but one of many
possible callers, but it's the one where one more check can make the
difference between a usable and a cryptic error for many cases.
(And there's no point in cluttering everything with checks as we're soon gonna have atomic support anyway, aren't we? ;-) )
Closes: #293
Testing procedure: Run the minimal example of the closed issue. Instead of the long errors, there's now a concise:
If it weren't for the front-end check in VisitTypedefNameDecl (and removing that was how I tested the back-end check), the error would be instead: