Page MenuHomePhabricator

[X86] Support of no_caller_saved_registers attribute (Clang part) - restart
ClosedPublic

Authored by oren_ben_simhon on Apr 9 2017, 11:04 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

oren_ben_simhon edited the summary of this revision. (Show Details)Apr 9 2017, 11:09 PM
aaron.ballman edited edge metadata.Apr 10 2017, 1:35 PM

Please remove the svn prop changes for the two test files. Also, I'd like to see a test that this is properly diagnosed as both a function and a type attribute on a non-x86 architecture, as well as tests that it is properly diagnosed on a type other than a function pointer as well as with arguments on a function pointer (the current tests only test on a declaration rather than a type).

include/clang/AST/Type.h
2934–2935 ↗(On Diff #94635)

You should read the comment above -- this goes over 10 bits, but you forgot to update the comment and Type::FunctionTypeBitfields.

include/clang/Basic/AttrDocs.td
2663–2666 ↗(On Diff #94635)

This appears to be copied word for word from GCC's documentation...

2664 ↗(On Diff #94635)

Single space instead of double space.

2670 ↗(On Diff #94635)

call clobbered -> call-clobbered

2673 ↗(On Diff #94635)

override -> overrides

2685 ↗(On Diff #94635)

on -> in
Also, missing a full stop at the end of the sentence.

2688 ↗(On Diff #94635)

register -> registers

include/clang/CodeGen/CGFunctionInfo.h
486 ↗(On Diff #94635)

I would remove this and make the ASTCallingConvention have 7 bits. Alternatively, scrape the unused bits from all of the fields and make this an unnamed bit-field.

569 ↗(On Diff #94635)

Missing a full stop at the end of the sentence.

lib/Sema/SemaDeclAttr.cpp
1947 ↗(On Diff #94635)

attr doesn't match the usual naming conventions (though don't name it Attr because that's the name of a type).

1964 ↗(On Diff #94635)

Same comment here.

oren_ben_simhon marked 11 inline comments as done.Apr 12 2017, 5:47 AM

aaron.ballman added a comment:
Please remove the svn prop changes for the two test files. Also, I'd like to see a test that this is properly diagnosed as both a function and a type attribute on a non-x86 architecture, as well as tests that it is properly diagnosed on a type other than a function pointer as well as with arguments on a function pointer (the current tests only test on a declaration rather than a type).

I removed the properties.
Please notice that the attribute is x86 specific.
Anyway, I added a test for non-x86 and also added more use-cases in the test.

I think all the suggested testcases are covered.
Please let me know if i missed anything. if so please provide an example to make sure i understand what you mean.

lib/Sema/SemaDeclAttr.cpp
1947 ↗(On Diff #94635)

I agree with you that "attr" doesn't follow the naming convention.

But I beg to differ regarding the use of "Attr".
I prefer to follow the grandfather coding guideline rule and use it like the neighboring functions do.

oren_ben_simhon marked an inline comment as done.

Implemented comments posted until 04/10 (Thank you Aaron)

aaron.ballman added inline comments.Apr 13 2017, 6:37 AM
include/clang/AST/Type.h
2945 ↗(On Diff #94959)

This offset looks incorrect -- according to the text above, the regparm offset should now be 8.

include/clang/CodeGen/CGFunctionInfo.h
486 ↗(On Diff #94635)

This didn't scrape all of the unused bits. For instance, EffectiveCallingConvention and CallingConvention appear to have unused bits still.

lib/Sema/SemaDeclAttr.cpp
1947 ↗(On Diff #94635)

This makes a bad problem worse, but in the end, it's not a huge issue -- the whole file could use a bit of churn to fix the problem. However, you should back out the unrelated changes to handleNoReturnAttr(). While I agree with them, they don't relate to the patch at hand.

test/SemaCXX/attr-non-x86-no_caller_saved_registers.cpp
1–4 ↗(On Diff #94959)

I think you really only need one RUN line; having four doesn't add value.

15 ↗(On Diff #94959)

Why is the same diagnostic triggering multiple times for the same line?

17 ↗(On Diff #94959)

The formatting here is wrong.

26 ↗(On Diff #94959)

Given that the attribute is not supported for any of the run lines, I am surprised at the expected error. That would make sense if the attribute were applied to foo, but it should not have been applied there. I think this is because the type attribute is not checking the target properly as the declaration attribute does (it happens automatically for declaration attributes, but not for type attributes).

test/SemaCXX/attr-x86-no_caller_saved_registers.cpp
1–5 ↗(On Diff #94959)

Why so many run lines? I think you only need one.

18 ↗(On Diff #94959)

Formatting is off.

24 ↗(On Diff #94959)

Why two different diagnostics for the same issue?

oren_ben_simhon marked 10 inline comments as done.Apr 18 2017, 4:55 AM

Implemented comments posted until 04/17 (Thank you Aaron for reviewing, Thank you Erich for helping me)

aaron.ballman added inline comments.Apr 19 2017, 5:45 AM
lib/Sema/SemaDecl.cpp
2962 ↗(On Diff #95551)

Is there a reason why this doesn't diagnose, such as ns_returns_retained and regparm attributes do?

lib/Sema/SemaDeclAttr.cpp
1972–1975 ↗(On Diff #95551)

Formatting here is off.

test/SemaCXX/attr-non-x86-no_caller_saved_registers.cpp
10 ↗(On Diff #95551)

This seems odd to me -- everywhere else this is an unknown attribute, except for arguments, where it suddenly is a known attribute that we diagnose. I think we want this to still be an unknown attribute in that case.

oren_ben_simhon marked 3 inline comments as done.Apr 19 2017, 8:29 AM

Implemented comments posted until 04/19 (Thank you Aaron again)

aaron.ballman added inline comments.Apr 19 2017, 8:54 AM
include/clang/Basic/DiagnosticSemaKinds.td
2804 ↗(On Diff #95759)

Please quote the attribute name in both places (the other cases should be quoted as well, but that isn't your patch's problem).

oren_ben_simhon marked an inline comment as done.Apr 20 2017, 3:26 AM

Implemented comments posted until 04/20 (Thanks Aaron)

aaron.ballman added inline comments.Apr 23 2017, 9:54 AM
include/clang/Basic/DiagnosticSemaKinds.td
2804 ↗(On Diff #95759)

This is a good, general diagnostic (we should eventually roll the other, similar diagnostics into this one, but that's a separate patch), but it's missing a test case to exercise it.

oren_ben_simhon marked an inline comment as done.

Implemented comments posted until 04/23 (10x Aaron)

include/clang/Basic/DiagnosticSemaKinds.td
2804 ↗(On Diff #95759)

Sure, I will add it in my next patch.

Hi Aaron, Please let me know if you have additional comments.

Hi Aaron, Please let me know if you have additional comments.

The missing test case is my last remaining issue with the patch.

Hi Aaron, Please let me know if you have additional comments.

The missing test case is my last remaining issue with the patch.

I added the test case in file test/SemaCXX/attr-x86-no_caller_saved_registers.cpp lines 22-24. Let me know if another test case is missing.

This revision is now accepted and ready to land.Apr 26 2017, 11:27 AM
This revision was automatically updated to reflect the committed changes.