This is an archive of the discontinued LLVM Phabricator instance.

[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–2936

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

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

2664

Single space instead of double space.

2670

call clobbered -> call-clobbered

2673

override -> overrides

2685

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

2688

register -> registers

include/clang/CodeGen/CGFunctionInfo.h
486

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.

566

Missing a full stop at the end of the sentence.

lib/Sema/SemaDeclAttr.cpp
1956

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

1964

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
1956

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

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

include/clang/CodeGen/CGFunctionInfo.h
486

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

lib/Sema/SemaDeclAttr.cpp
1956

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
2–5

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

16

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

18

The formatting here is wrong.

27

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
2–6

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

19

Formatting is off.

25

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

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

lib/Sema/SemaDeclAttr.cpp
1972–1975

Formatting here is off.

test/SemaCXX/attr-non-x86-no_caller_saved_registers.cpp
10

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.