Skip to content
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

GetCacheSizesMacOSX(): use consistent types. #667

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

LebedevRI
Copy link
Collaborator

I have absolutely no way to test this, but this looks obviously-good.

This was reported by Tim Northover @TNorthover in
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180903/584223.html

I think this breaks some 32-bit configurations (well, mine at least).
I was using Clang (from Xcode 10 beta) on macOS and got a bunch of
errors referencing sysinfo.cc:292 and onwards:

/Users/tim/llvm/llvm-project/llvm/utils/benchmark/src/sysinfo.cc:292:47:
error: non-constant-expression cannot be narrowed from type
'std::__1::array<unsigned long long, 4>::value_type' (aka 'unsigned
long long') to 'size_t' (aka 'unsigned long') in initializer list
[-Wc 11-narrowing]
} Cases[] = {{"hw.l1dcachesize", "Data", 1, CacheCounts[1]},
^~~~~~~~~~~~~~

The same happens when self-hosting ToT. Unfortunately I couldn't
reproduce the issue on Debian (Clang 6.0.1) even with libc ; I'm not
sure what the difference is.

I have absolutely no way to test this, but this looks obviously-good.

This was reported by Tim Northover @TNorthover in
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180903/584223.html

> I think this breaks some 32-bit configurations (well, mine at least).
> I was using Clang (from Xcode 10 beta) on macOS and got a bunch of
> errors referencing sysinfo.cc:292 and onwards:

> /Users/tim/llvm/llvm-project/llvm/utils/benchmark/src/sysinfo.cc:292:47:
> error: non-constant-expression cannot be narrowed from type
> 'std::__1::array<unsigned long long, 4>::value_type' (aka 'unsigned
> long long') to 'size_t' (aka 'unsigned long') in initializer list
> [-Wc  11-narrowing]
>   } Cases[] = {{"hw.l1dcachesize", "Data", 1, CacheCounts[1]},
>                                               ^~~~~~~~~~~~~~
>
> The same happens when self-hosting ToT. Unfortunately I couldn't
> reproduce the issue on Debian (Clang 6.0.1) even with libc  ; I'm not
> sure what the difference is.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.635% when pulling cc9cd96 on LebedevRI:llvm-osx into fbfc495 on google:master.

@TNorthover
Copy link

Thanks Roman! I checked on my local build and this did indeed fix the issue.

@AppVeyorBot
Copy link

Build benchmark 1406 failed (commit e65c8d5ece by @LebedevRI)

@dmah42
Copy link
Member

dmah42 commented Sep 4, 2018

The test bots do a pretty good job of covering 32-bit and such, but unfortunately didn't catch this one. We must be missing a configuration.

@LebedevRI
Copy link
Collaborator Author

There are no 32-bit builds for OSX as far as i can tell.
Though i'm unfamiliar with that target so i'm unsure i can help adding them..

LebedevRI added a commit to LebedevRI/benchmark that referenced this pull request Sep 5, 2018
Maybe will show google#667
or maybe this is completely wrong.
@LebedevRI
Copy link
Collaborator Author

LebedevRI commented Sep 5, 2018

@dominichamon

We must be missing a configuration.

Yep, it turned out to be trivial #669
Looks good now?

dmah42 pushed a commit that referenced this pull request Sep 5, 2018
Maybe will show #667
or maybe this is completely wrong.
@dmah42 dmah42 merged commit f090141 into google:master Sep 5, 2018
@LebedevRI LebedevRI deleted the llvm-osx branch September 5, 2018 11:21
@LebedevRI
Copy link
Collaborator Author

I wanted to rebase this to double-check that it actually is fully sufficient, but i suppose a merge works, too :)
Thanks!

JBakamovic pushed a commit to JBakamovic/benchmark that referenced this pull request Dec 6, 2018
Maybe will show google#667
or maybe this is completely wrong.
JBakamovic pushed a commit to JBakamovic/benchmark that referenced this pull request Dec 6, 2018
I have absolutely no way to test this, but this looks obviously-good.

This was reported by Tim Northover @TNorthover in
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180903/584223.html

> I think this breaks some 32-bit configurations (well, mine at least).
> I was using Clang (from Xcode 10 beta) on macOS and got a bunch of
> errors referencing sysinfo.cc:292 and onwards:

> /Users/tim/llvm/llvm-project/llvm/utils/benchmark/src/sysinfo.cc:292:47:
> error: non-constant-expression cannot be narrowed from type
> 'std::__1::array<unsigned long long, 4>::value_type' (aka 'unsigned
> long long') to 'size_t' (aka 'unsigned long') in initializer list
> [-Wc  11-narrowing]
>   } Cases[] = {{"hw.l1dcachesize", "Data", 1, CacheCounts[1]},
>                                               ^~~~~~~~~~~~~~
>
> The same happens when self-hosting ToT. Unfortunately I couldn't
> reproduce the issue on Debian (Clang 6.0.1) even with libc  ; I'm not
> sure what the difference is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants