User Details
- User Since
- Nov 13 2015, 7:20 AM (271 w, 2 d)
Sun, Jan 10
I investigated this issue a bit under debugger. It seems that there are multiple places where resolver is used as "base" object for ifunc and resolver attributes like comdat are applied to ifunc itself. It is reasonable for aliases but not for ifunc. Unfortunately it is not something that can be changed easily. The proposal with deriving GlobalIFunc from GlobalObject makes more sense to me now.
Sat, Jan 9
Oh, it seems that LTO is already relay on getBaseObject from ifunc :(
Wed, Jan 6
I think it should fix immediate problem that we have and makes things more consistent in general.
Nov 18 2020
@njames93 thank you for the feedback!
- addressed comments in the diff
- stop ignoring system macro
Nov 5 2020
Fix clang-tidy warning
Nov 4 2020
Sorry, I don't think it's worth reverting and resubmitting the same changes in such a case. Commit attribution is correct and there is link to this code review.
After all, most clang-tidy users will never read commit messages and will only use --help for discovering this feature.
Nov 3 2020
Hello @@DmitryPolukhin ,
When I submitted latest via 'arc diff' my commit-message was...
Can you please help to have following commit message? Below commit message is more clear and helpful.
Sorry for inconvenience caused!
Oct 30 2020
Looks good to me.
Oct 29 2020
Oct 28 2020
I think this diff looks very close to what we need. I hope it will be the last iteration.
Oct 27 2020
Yes, it is what I meant as a simpler solution. But please add Lit test even for such trivial option.
@Hiralo, it looks like you uploaded wrong diff because your previous revision is shown as "base" version. Base revision should be clang-tidy sources without your changes.
Oct 26 2020
I'm not sure that we need additional option to read configuration from file but, if we do need, I think this diff needs some improvements + test for new option.
Oct 23 2020
Oct 22 2020
Added all module map file names
Oct 21 2020
And one more time linting
Linting diff
Jul 29 2020
Thank you for identifying and fixing!
Jul 9 2020
@aaron.ballman - thank you for the review!
Jul 8 2020
I don't understand what do you mean by "not idempotent" behavior in this case. As far as I can see GlobalIFunc doesn't implement own getBaseObject (and it is not virtual) so calling getBaseObject on the IFunc should return null same as calling it on Alias-to-IFunc. Calling getbaseObject on Alias-to-IFunc will recursively call it on IFunc that will return null that will be propagated, isn't it? So in my opinion computeAliasSummary should handle null without crash because other places have checks for null returned from getBaseObject.
Jul 7 2020
I did a bit of archeology and it turns out that getBaseObejct was part of moved from GlobalAlias to GlobalIndirectSymbol in https://github.com/llvm/llvm-project/commit/95549497ec8b5269f0439f12859537b7371b7c90
It looks like the simplest solution is to handle nullptr from getBaseObejct in computeAliasSummary...
getBaseObject is only small part of code sharing, the majority of the code is outside the class in common code that handles both GlobalIFunc and GlobalAliases in the same way. Here they should be treated differently so, IMHO, there is no need in changing the inheritance but GlobalIFunc should be handles as a special case here.
Jul 6 2020
@njames93 and @aaron.ballman - please take a look to this diff. Multiline replacements in YAML are broken and cannot be applied correctly.
Jun 24 2020
Fix test
Use double quotation for multiline strings. It solves problems with internal newlines because now they are escaped. Also double quotation solves the problem with leading whitespace after newline. In case of single quotation YAML parsers should remove leading whitespace according to specification. In case of double quotation these leading are internal space and they are preserved. There is no way to instruct YAML parsers to preserve leading whitespaces after newline so double quotation is the only viable option that solves all problems at once.
Jun 19 2020
It looks like there is no support for the proposed solution so I found alternative solution that might be even better. We can use double quotation " for multiline strings. It solves problem because in case of double quotation LLVM escapes new line like \n so there is no need to double newlines. Escaped newlines can be parsed correctly by other YAML parsers like pyyaml. BTW, LLVM YAML reading also has issue with not removing leading spaces for multiline strings so multiline strings serialised by pyyaml with single quotation cannot be parsed correctly by clang-apply-replacements. With double quotation it seems to work fine in both directions.
Jun 18 2020
@njames93 - friendly ping, could you please take another look.
Jun 16 2020
Rebase
LGTM
Jun 15 2020
@aaron.ballman thank you for review and quick response!
@alexfh, @gribozavr2, @aaron.ballman - friendly ping, please take a look to this diff. It is just trivial implementation of TODO in the code.
Jun 12 2020
Rebase
Jun 11 2020
Fix single new line handling, it should be replace with space
Jun 5 2020
Fix spelling
Rewrite input string split too
Jun 4 2020
Apply suggested changes with string split
Jun 2 2020
+ @gribozavr, @Eugene.Zelenko, @thegameg who touched/reviewed this code, please take a look.
May 20 2020
Fixed second string after new line + more test case
Fix clang-tidy warnings
Sent D80301 for review.
@mgehre @yvvan it seems that the issue still not fixed and '\n' gets duplicated in the replacements. Are you going to fix this issue or I should create a patch to fix it?
Before this change '\n' was actually processed correctly ReplacementText: '#include <utility>\n\n' is actually replacement text with two new lines in standard YAML deserialiser.
May 15 2020
@alexfh and @gribozavr2 friendly ping could you please take a look
May 2 2020
Apr 17 2020
Apr 16 2020
Comments resolved
Split TypeOffsets to low/high parts too
Apr 15 2020
@thakis, I don't see this bot on LLVM http://lab.llvm.org:8011/console
Windows bots there still fail due to cmake issues. The issue is very real and thank you for pointing out but how should I find the bot?
@thakis sorry, I didn't notice the issue because of massive breakage with diff landed just be bore mine and also cmake issues on Windows bots. Thank you for trying to fix it.
@sammccall thank you for review!
I'll wait for one more day for additional feedback if any, and land this diff.
Fix nit
Split BitOffset in DeclOffset in high/low parts; rebase
Split BitOffset in DeclOffset in high/low parts; rebase
Apr 14 2020
And one more time :(
One more try to remove useless lint issues
Remove clang-format errors in diff
Comments resolved + rebase
@alexfh thank you for review!
Apr 13 2020
@alexfh friend ping, please take a look.
Apr 2 2020
@rsmith, @dexonsmith, @jdoerfert could you please take a look to this diff?
If you think that there are other reviewers who might have more context AST persistence, please add them.
@alexfh friendly ping
I implemented solution with priorities to resolve your concerns about get() vs getLocalOrGlobal()
Could you please take another look to this diff?
Apr 1 2020
Use options priority instead of overriding local options by global