This is an archive of the discontinued LLVM Phabricator instance.

[clang][github] Added checking for completeness of lvalue in conditional operator [#59718]
ClosedPublic

Authored by Aditya-pixel on Feb 19 2023, 9:05 PM.

Details

Summary

[ISSUE] https://github.com/llvm/llvm-project/issues/59718

struct x y;
int main(void)
{
    (void)(1 ? y : y);
}
struct x {int i;};

The conditional operator(?:) requires the second and third operands to be of compatible types. To be compatible, they also need to be complete (however, both can be void). Therefore, the expected response from clang after running the above code as a C program should be error dialogue pointing out that both the types are incomplete hence incompatible, but the code compiled without any errors.
The patch ensures the completeness in the CheckCondtionalOperand function present in llvm-project/clang/lib/Sema/SemaChecking.cpp

Diff Detail

Event Timeline

Aditya-pixel created this revision.Feb 19 2023, 9:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2023, 9:05 PM
Aditya-pixel requested review of this revision.Feb 19 2023, 9:05 PM

Hi, thanks for the patch, but AFAICT it at least has issues below:

  1. You need to add a regression test, probably in clang/test/Sema.
  2. You may need to update the release note, in clang/docs/ReleaseNotes.rst.
  3. You may need to format your patch, see more details at https://llvm.wiki/llvm/overview/#submit-a-patch
  4. Would it be better if you could add a comment on it?
shafik added a reviewer: Restricted Project.Feb 20 2023, 1:22 PM
shafik added a subscriber: shafik.

Thank you for taking the time to submit a patch.

In addition to the previous comments, please also add a summary of the problem and why this is a fix for it.

It would also be good to reference the github bug, I usually add a link some folks just add the issue number.

Thank you for all the suggestions and guidance. I will make the changes in the revision and also update the clang tests for Sema.

Aditya-pixel retitled this revision from Added checking for completeness of lvalue [#59718] to [clang][github] Added checking for completeness of lvalue in conditional operator [#59718].Feb 21 2023, 10:10 AM
Aditya-pixel edited the summary of this revision. (Show Details)
Aditya-pixel added a project: Restricted Project.

Hello, can someone help me in writing the tests in clang/test/Sema? I understand we have to add a test file in C but where should the expected diagnose be added? If there is a tutorial or note for it please comment the link or path. Thanks!

Hello, can someone help me in writing the tests in clang/test/Sema? I understand we have to add a test file in C but where should the expected diagnose be added? If there is a tutorial or note for it please comment the link or path. Thanks!

I don't think we have a good tutorial for adding tests to Clang (the tests kind of "go where they belong" which is a bit hard to define, as you might imagine!). In this case, I would add the test coverage to test/Sema/incomplete-decl.c because that test is about incomplete declarations in general. You might also want to add a second RUN line to that file which passes -x c++ to test the behavior in C++ as well as C, given that there's a difference in behavior between the two.

clang/lib/Sema/SemaChecking.cpp
14372–14375

You might as well use RequireCompleteExprType(E, diag::err_incomplete_type) as it's slightly simpler but does the same thing.

Hello, can someone help me in writing the tests in clang/test/Sema? I understand we have to add a test file in C but where should the expected diagnose be added? If there is a tutorial or note for it please comment the link or path. Thanks!

I don't think we have a good tutorial for adding tests to Clang (the tests kind of "go where they belong" which is a bit hard to define, as you might imagine!). In this case, I would add the test coverage to test/Sema/incomplete-decl.c because that test is about incomplete declarations in general. You might also want to add a second RUN line to that file which passes -x c++ to test the behavior in C++ as well as C, given that there's a difference in behavior between the two.

Hi! Thanks for the help. I found a nice documentation about regression tests which is quite detailed. I'll edit the code to RequireCompleteExprType(E, diag::err_incomplete_type) and run the tests.

Hello! I have updated the patch and added tests for C and C++. The tests are compatible and code is working as expected. Please do let me know if any changes are to be done or tests needs to be increased. Thank you.

aaron.ballman added inline comments.Mar 7 2023, 8:23 AM
clang/lib/Sema/SemaChecking.cpp
14373–14375

Our coding style would have you drop the curly braces here.

clang/test/Sema/incomplete-decl.c
2–4 ↗(On Diff #502384)

We typically try to keep #if conditions like this out of test code because it makes it too easy to not know what part of the test is actually being tested. There's a different approach that I think will work better here: use -verify=cxx on the C++ RUN line.

-verify turns on the diagnostic verifier, which looks for those expected-whatever comments. When you use -verify=foo, the diagnostic verifier looks for foo-whatever instead of expected-whatever. So if you use -verify=cxx, you can specify the C++-specific diagnostics with cxx-error {{words}} and the C-specific ones with expected-error {{different words}} which lets you get rid of the #if.

Hello! I tried this but I am getting some errors. I think the c++ RUN line is also checking the earlier defined test cases and throwing ['error' diagnostics seen but not expected.] Would should I do? Thanks.

Hello! I tried this but I am getting some errors. I think the c++ RUN line is also checking the earlier defined test cases and throwing ['error' diagnostics seen but not expected.] Would should I do? Thanks.

You can use multiple prefixes with -verify, so if you have some diagnostics in C++, some in C, and some in both, I usually do:

// RUN: %clang_cc1 -fsyntax-only -verify=expected, c %s
// RUN: %clang_cc1 -fsyntax-only -x c++ -verify=expected, cxx %s

void bad_code(); // c-warning {{this is bad in C}}
void more_bad_code(void); // cxx-warning {{this is bad in C++}}
void wow_this_is_horrible(float); // expected-warning {{this is bad in both C and C++}}

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

PS: How do I take care of code where both C++ and C throw error but error messages are different? Here's an example:
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: tentative definition has type 'struct foo' that is never completed - expected in C
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: variable has incomplete type 'struct foo' - error thrown by C++.
Thank you.

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

PS: How do I take care of code where both C++ and C throw error but error messages are different? Here's an example:
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: tentative definition has type 'struct foo' that is never completed - expected in C
File ./llvm-project/clang/test/Sema/incomplete-decl.c Line 9: variable has incomplete type 'struct foo' - error thrown by C++.
Thank you.

Good question! I handle that like this:

struct foo bad_code; // c-warning {{tentative definition has type 'struct foo' that is never completed}} \
                        cxx-warning {{variable has incomplete type 'struct foo'}}

(Note, I'm using a line continuation character (\) to continue the comment from one line to the next so the comments are both associated with the same source line.)

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

Sure, Here's the code and the llvm-lit results is commented below it. Thanks.
https://pastebin.com/vYpeDjmt#D74b9U7z

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

Sure, Here's the code and the llvm-lit results is commented below it. Thanks.
https://pastebin.com/vYpeDjmt#D74b9U7z

The trouble is that the comments for verifying the diagnostics aren't using the new prefix you added (and you missed adding the c prefix to that RUN line).

The run lines should be:

// RUN: %clang_cc1 -fsyntax-only -verify=c,expected %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=cxx,expected %s

and the comments should be more like:

static void c; // expected-error {{variable has incomplete type 'void'}}
static struct foo g;  // cxx-warning {{tentative definition of variable with internal linkage has incomplete non-array type 'struct foo'}} \
    c-error{{tentative definition has type 'struct foo' that is never completed}}

Where expected-whatever is being checked for both RUN lines, c-whatever is being checked only for the first RUN line, and cxx-whatever is being checked only for the second RUN line.

Hello! I had tried that as well and it does resolve some errors but some errors differ between c and c++. Some places C++ throws an error and not C, and vice-versa. Could I then add the C++ test cases to the previously defined tests? Thank you.

The places that issue an error in C++ but not C would use cxx-error {{...}} instead of c-error {{}} or expected-error {{}}. I think it'd help to see what your test looks like now and a pastebin (or something) of the output from lit.

Sure, Here's the code and the llvm-lit results is commented below it. Thanks.
https://pastebin.com/vYpeDjmt#D74b9U7z

The trouble is that the comments for verifying the diagnostics aren't using the new prefix you added (and you missed adding the c prefix to that RUN line).

The run lines should be:

// RUN: %clang_cc1 -fsyntax-only -verify=c,expected %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=cxx,expected %s

and the comments should be more like:

static void c; // expected-error {{variable has incomplete type 'void'}}
static struct foo g;  // cxx-warning {{tentative definition of variable with internal linkage has incomplete non-array type 'struct foo'}} \
    c-error{{tentative definition has type 'struct foo' that is never completed}}

Where expected-whatever is being checked for both RUN lines, c-whatever is being checked only for the first RUN line, and cxx-whatever is being checked only for the second RUN line.

Thanks I understand now, I'll make the appropriate changes in the testing file.

This comment was removed by Aditya-pixel.

UPDATED: Removed the initial use of #ifdef and added separate tests for C and C++. Here c-<diagnostic> means diagnostic is specific to C, cxx-<diagnostic> means diagnostic is specific to c++ and expected-<diagnostic> means diagnostic is common to both.

Can you run git clang-format HEAD~1 to format your patch? (Put https://github.com/llvm/llvm-project/blob/main/clang/tools/clang-format/git-clang-format in your PATH, like ~/.local/bin the command will work)

Hello! I have added the diff file after running the clang-format command. However, it outputs no files changed. Is the patch I updated formatted?


I am getting this message.

Hello! I have added the diff file after running the clang-format command. However, it outputs no files changed. Is the patch I updated formatted?


I am getting this message.

I don't see any formatting issues in the patch you've uploaded, and precommit CI didn't find any concerns either, so I think you're fine.

clang/test/Sema/incomplete-decl.c
7–9 ↗(On Diff #506853)

cxx-note 3 {{forward declaration of 'foo'}} instead of duplicating the same diagnostic across multiple lines?

Also, to help make it easier to read, can you indent the second line so that cxx-note aligns with c-note from the line above? (Same suggestion throughout the file.)

UPDATED: Formatted the file, added indents and condensed comments.

aaron.ballman accepted this revision.Mar 24 2023, 6:23 AM

LGTM! (The changes need a release note, though.) Do you need someone to commit this on your behalf? If so, please let me know what name and email address you would like used for patch attribution. I can add the release note when I land, or you can update the patch to add a release note, either is fine by me.

This revision is now accepted and ready to land.Mar 24 2023, 6:23 AM

LGTM! (The changes need a release note, though.) Do you need someone to commit this on your behalf? If so, please let me know what name and email address you would like used for patch attribution. I can add the release note when I land, or you can update the patch to add a release note, either is fine by me.

Hello! If it's not an inconvenience you could commit the changes and add the release notes.
Name: Aditya Singh
Email Address: f20190478@goa.bits-pilani.ac.in
Thanks a lot.

UPDATE: Added release note, please have a look and suggest if anything more needs to be mentioned. Thanks.

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

clang/docs/ReleaseNotes.rst
137 ↗(On Diff #508433)

This should be:

(`#59718 <https://github.com/llvm/llvm-project/issues/59718>`_)

with the trailing _.

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

I'll fix that one up and land it as an NFC change, so you don't need to worry about that. Thanks for pointing it out though!

I tried to land this for you today but I can't apply the patch locally because there's no patch context for git to use to figure out where to apply the changes. Can you rebase and make a new diff using -U999 to add a bunch of extra context to the patch, and upload that? I should be able to land it for you then.

The trailing '_' is not in the notes above as well, should I add the trailing _ here too?

-We now generate a diagnostic for signed integer overflow due to unary minus 
 in a non-constant expression context.
 (`#31643 <https://github.com/llvm/llvm-project/issues/31643>`)

I'll fix that one up and land it as an NFC change, so you don't need to worry about that. Thanks for pointing it out though!

Ah that one was already fixed it seems: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst?plain=1#L171

Oh, okay, Ill change the patch now. Also, it seems my local directory is out of sync. Should I always update it before submitting a patch, or is it alright without updating it?

This revision was landed with ongoing or failed builds.Mar 27 2023, 11:47 AM
This revision was automatically updated to reflect the committed changes.

Oh, okay, Ill change the patch now. Also, it seems my local directory is out of sync. Should I always update it before submitting a patch, or is it alright without updating it?

It's usually good to rebase each time you update the patch, but it's not a requirement either (sometimes it's easier to rebase at the end instead of while the patch is under review). I've landed the changes for you in dedd7b6548f4a37f4f691aa0cd3a709756b7e794