Use the cpuid instruction to generate the CPU brand string. And

Use the cpuid instruction to generate the CPU brand string. And 
use this string to check whether the cpu is Intel Atom processor.

This change is to make sure the 2D canvas acceleration is enabled
on Atom / windows system.

BUG=151500
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160226

[Zhenyao Mo, 2012-10-01 19:04:33]

http://codereview.chromium.org/11009011/diff/4002/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/4002/base/cpu.cc#newcode123
base/cpu.cc:123: //get the brand string of the cpu.
one space after //

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
File content/browser/gpu/gpu_blacklist.cc (right):

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
content/browser/gpu/gpu_blacklist.cc:19: #include "base/cpu.h"
You need to insert the include in the right place according to alphabetic
ordering

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
content/browser/gpu/gpu_blacklist.cc:582: const DictionaryValue* cpu_info_value
= NULL;
cpu_brand_value, here and below.

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
content/browser/gpu/gpu_blacklist.cc:1074: base::CPU cpu_info;
indent two spaces instead of four.

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
content/browser/gpu/gpu_blacklist.cc:1076: return false;
indent two spaces

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
File content/browser/gpu/gpu_blacklist.h (right):

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
content/browser/gpu/gpu_blacklist.h:363: bool SetCpuInfo(const std::string&
cpu_op,
SetCpuBrand is better

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
content/browser/gpu/gpu_blacklist.h:364: const std::string& cpu_value);
too many spaces

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/gpu_bla...
content/browser/gpu/gpu_blacklist.h:415: scoped_ptr<StringInfo> cpu_info_;
cpu_brand_

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/softwar...
File content/browser/gpu/software_rendering_list.json (right):

http://codereview.chromium.org/11009011/diff/4002/content/browser/gpu/softwar...
content/browser/gpu/software_rendering_list.json:461: "exceptions": [
indent -1

[Zhenyao Mo, 2012-10-02 22:01:31]

LGTM with two nits

http://codereview.chromium.org/11009011/diff/10001/base/cpu.h
File base/cpu.h (right):

http://codereview.chromium.org/11009011/diff/10001/base/cpu.h#newcode35
base/cpu.h:35: const std::string& cpu_brand() const { return cpu_brand_; }
nit: you need an empty line before private: (sorry I didn't catch this the first
round)

http://codereview.chromium.org/11009011/diff/10001/content/browser/gpu/gpu_bl...
File content/browser/gpu/gpu_blacklist.cc (right):

http://codereview.chromium.org/11009011/diff/10001/content/browser/gpu/gpu_bl...
content/browser/gpu/gpu_blacklist.cc:9: #include "base/cpu.h"
nit: still wrong order, move one line up

[Zhenyao Mo, 2012-10-02 22:32:18]

http://codereview.chromium.org/11009011/diff/14001/base/cpu.h
File base/cpu.h (right):

http://codereview.chromium.org/11009011/diff/14001/base/cpu.h#newcode36
base/cpu.h:36: 
nit: you have empty spaces in this empty.  see lint complaints.

[jar (doing other things), 2012-10-03 00:33:04]

Drive by comments.

I don't know if the bug about null termination is real (I don't know about these
structures), but the nits would help make it easier spot if it was.

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode87
base/cpu.cc:87: char cpu_string[0x20];
Here this is declared to be length 32, but on lines 101-103, we only use 12
bytes, and perhaps counting on null trailing bytes at offset 13 to terminate the
string.  IMO, it would be helpful to use decimal, and this might be clearer.

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode88
base/cpu.cc:88: char cpu_brand[0x30];
Please use decimal, rather than hex here.  48 is much clearer.

Given that the code writes 48 bytes, it would make sense to add at least one
extra byte with a null, since you're going to assume it is null terminated on
line 137.

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode102
base/cpu.cc:102: *(reinterpret_cast<int*>(cpu_string+4)) = cpu_info[3];
nit: white space around operator "+".

Better yet IMO would be to use a subscript operator (as suggested for the new
code).

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode132
base/cpu.cc:132: *(reinterpret_cast<int*>(cpu_brand + index*16)) = cpu_info[0];
nit: spaces around an arithmetic operator "*"

Better yet IMO, would be to use the subscript operator, which is much clearer
than pointer arithmetic.

See comment below for having the nicer choice of types.

You could probably also do this nicer with a nested loop, rather than repeating
the same line (almost) on lines 132-135.

Note that on lines 101-103 they are doing some permuting (perhaps
bigendian/littleendian isuses??), but you're just copying in order here (I
think).

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode137
base/cpu.cc:137: cpu_brand_ = cpu_brand;
How about declaring cpu_brand to be:
int cup_brand[12];
Then you'd only have one (sad) cast:
cpu_brand_ = reinterpret_cast<char*>(cpu_brand);

http://codereview.chromium.org/11009011/diff/11007/content/browser/gpu/gpu_bl...
File content/browser/gpu/gpu_blacklist.cc (right):

http://codereview.chromium.org/11009011/diff/11007/content/browser/gpu/gpu_bl...
content/browser/gpu/gpu_blacklist.cc:867: new StringInfo(cpu_op, cpu_value));
nit: no reason to wrap this line.

[Mark Mentovai, 2012-10-03 00:33:17]

You picked too many reviewers to spam. You don’t need to name all of the OWNERS
in a directory. I fixed that for you.

Are you listed in the AUTHORS file?

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode128
base/cpu.cc:128: int start_flag = 0x80000002;
“flag” isn’t a good name. Thcpuid documentation usually calls the eax parameter
to cpuid a “function,” but “parameter” or “argument” would be acceptable too.

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode130
base/cpu.cc:130: for (int index = 0; index < 3; index++) {
Why don’t you just write the loop by initializing the loop variable to
0x80000002 and testing that it <= 0x80000004?

Then, 0x80000004 can be a const int, and you can use it in the check on line 126
as well.

http://codereview.chromium.org/11009011/diff/11007/base/cpu.cc#newcode132
base/cpu.cc:132: *(reinterpret_cast<int*>(cpu_brand + index*16)) = cpu_info[0];
I can see that this was copied from line 101, but this isn’t great. This type
punning breaks strict aliasing rules. It’s also a little silly to do these four
operations when a single memcpy would suffice. memcpy is also fine even under
strict aliasing rules.

You should fix up the other code in this file as well.

[Mark Mentovai, 2012-10-03 18:34:02]

http://codereview.chromium.org/11009011/diff/26001/AUTHORS
File AUTHORS (right):

http://codereview.chromium.org/11009011/diff/26001/AUTHORS#newcode174
AUTHORS:174: Dai Chunyang <chunyang.dai@intel.com>
End of the file, please.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode87
base/cpu.cc:87: char cpu_string[32];
Now let’s make this stuff conform to Chromium style a little better. Don’t
declare variables until they’re used.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode88
base/cpu.cc:88: char cpu_brand[48];
You can share a buffer for cpu_string and cpu_brand as long as it’s sized
appropriately, since the two buffers aren’t both needed at the same time.

If you share the buffer, you obviously won’t be able to use sizeof(BUF) where I
recommend you do that below, but the alternatives should be very obvious.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode89
base/cpu.cc:89: const int para_end = 0x80000004;
Don’t abbreviate.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode97
base/cpu.cc:97: // http://msdn.microsoft.com/en-us/library/hskdteyh.aspx
This URL is no longer valid. You should remove it, unless you can find a current
URL, in which case you can revise it.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode101
base/cpu.cc:101: *(reinterpret_cast<int*>(cpu_string)) = cpu_info[1];
Fix these existing broken assignments to use memcpy as well, although
unfortunately the string is stored non-contiguously in cpu_info, so you will
need EITHER three memcpys (meh) OR you’ll need to std::swap ecx with edx
(cpu_info[3] with cpu_info[2]) and do a single memcpy. You can get rid of the
memset.

Put spaces around the + operators when you make this change.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode103
base/cpu.cc:103: *(reinterpret_cast<int*>(cpu_string+8)) = cpu_info[2];
cpu_string is char[32] but it only needs to be char[12]. However, when you do
that, you lose the NUL terminator. To address that, you should change line 114
from |cpu_vendor_ = cpu_string;| to |cpu_vendor_.assign(cpu_string,
sizeof(cpu_string));|.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode114
base/cpu.cc:114: cpu_vendor_ = cpu_string;
This doesn’t seem to require num_ids to be > 0. You can move this out of the
|num_ids > 0| conditional. You should set cpu_vendor_ as soon as you’ve gotten
cpu_string worked out.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode123
base/cpu.cc:123: // get the brand string of the cpu.
Comments should be written as sentences, and sentences start with capital
letters.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode123
base/cpu.cc:123: // get the brand string of the cpu.
I don’t think there’s any reason for this to be in the |num_ids > 0|
conditional. The “basic” and “extended” groups of cpuid are totally distinct.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode127
base/cpu.cc:127: memset(cpu_brand, 0, sizeof(cpu_brand));
You don’t need this memset, because you’re going to memcpy over cpu_brand
anyway.

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode135
base/cpu.cc:135: cpu_brand_ = cpu_brand;
cpu_brand is not necessarily NUL-terminated as you’ve written things here
(although cpuid is SUPPOSED to return a NUL-terminated string, I wouldn’t rely
on that). See the comment above about NUL-termination for cpu_string, and
address this in the same way that you deal with that.

[chunyang.dai, 2012-10-04 13:32:31]

On 2012/10/03 18:34:02, Mark Mentovai wrote:

I submit a new patch basing on Mark's comments. 
Thanks a lot for your review.

[Mark Mentovai, 2012-10-04 15:51:28]

zmo should also re-review the base portion since it’s changed substantially
since his review.

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc#newcode94
base/cpu.cc:94: // CPUInfo[3] | CPUInfo[2]. So we swap the CPUInfo[2] and
CPUInfo[3]
Don’t write “we” in comments. This sentence reads more clearly if you remove the
“we.” And still more clearly if you also remove the “so.”

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc#newcode95
base/cpu.cc:95: // before memcpy them to cpu_string.
memcpy is not a verb.

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc#newcode98
base/cpu.cc:98: std::swap(cpu_info[2],cpu_info[3]);
Space after comma.

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc#newcode99
base/cpu.cc:99: memcpy(cpu_string, &cpu_info[1], 3*sizeof(int));
Space around operators. Please familiarize yourself with the style guide.

[Mark Mentovai, 2012-10-04 16:43:53]

LGTM with this change for base OWNERS approval. I only reviewed the portion in
base. zmo should still have another look as I mentioned earlier.

http://codereview.chromium.org/11009011/diff/27002/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/27002/base/cpu.cc#newcode98
base/cpu.cc:98: std::swap(cpu_info[2], cpu_info[3]);
#include <algorithm> to use std::swap.

[Zhenyao Mo, 2012-10-04 16:53:15]

LGTM

[chunyang.dai, 2012-10-04 17:11:23]

I submit the #include <algorithm> sentence accordig to Mark's comments.  
And thank you all for your review.

[commit-bot: I haz the power, 2012-10-04 17:14:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chunyang.dai@intel.com/11009011/32003

[commit-bot: I haz the power, 2012-10-04 17:14:11]

Failed to apply patch for content/browser/gpu/software_rendering_list.json:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file content/browser/gpu/software_rendering_list.json
  Hunk #2 FAILED at 74.
  Hunk #3 succeeded at 474 (offset 16 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
content/browser/gpu/software_rendering_list.json.rej

Patch:       content/browser/gpu/software_rendering_list.json
Index: content/browser/gpu/software_rendering_list.json
===================================================================
--- content/browser/gpu/software_rendering_list.json	(revision 159378)
+++ content/browser/gpu/software_rendering_list.json	(working copy)
@@ -36,19 +36,20 @@
 // 15. "machine_model" contais "name" and an optional "version".  "name" is a 
 //     STRING structure and "version" is a VERSION structure (defined below).
 // 16. "gpu_count" is a INT structure (defined below).
-// 17. "exceptions" is a list of entries.
-// 18. "blacklist" is a list of gpu feature strings, valid values include
+// 17  "cpu_info" is a STRING structure (defined below).
+// 18. "exceptions" is a list of entries.
+// 19. "blacklist" is a list of gpu feature strings, valid values include
 //     "accelerated_2d_canvas", "accelerated_compositing", "webgl",
 //     "multisampling", "flash_3d", "flash_stage3d", "texture_sharing",
 //     "accelerated_video_decode", and "all".
 //     This field is mandatory.
-// 19. "description" has the description of the entry.
-// 20. "webkit_bugs" is an array of associated webkit bug numbers.
-// 21. "cr_bugs" is an array of associated webkit bug numbers.
-// 22. "browser_version" is a VERSION structure (defined below).  If this
+// 20. "description" has the description of the entry.
+// 21. "webkit_bugs" is an array of associated webkit bug numbers.
+// 22. "cr_bugs" is an array of associated webkit bug numbers.
+// 23. "browser_version" is a VERSION structure (defined below).  If this
 //     condition is not satisfied, the entry will be ignored.  If it is not
 //     present, then the entry applies to all versions of the browser.
-// 23. "disabled" is a boolean. If it is present, the entry will be skipped.
+// 24. "disabled" is a boolean. If it is present, the entry will be skipped.
 //     This can not be used in exceptions.
 //
 // VERSION includes "op", "style", "number", and "number2".  "op" can be any of
@@ -73,7 +74,7 @@
 {
   "name": "software rendering list",
   // Please update the version number whenever you change this file.
-  "version": "3.4",
+  "version": "3.5",
   "entries": [
     {
       "id": 1,
@@ -457,6 +458,14 @@
         "op": "<",
         "value": "3.5"
       },
+      "exceptions": [
+        {
+          "cpu_info": {
+            "op": "contains",
+            "value": "Atom"
+          }
+        }
+      ],
       "blacklist": [
         "accelerated_2d_canvas"
       ]

[jar (doing other things), 2012-10-04 17:42:02]

More drive by...

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode135
base/cpu.cc:135: cpu_brand_ = cpu_brand;
I couldn't find documentation that suggests the string comes null terminated. 
The sample documentation provides sample code that does the null termination,
and worse yet, the example given shows the CPU_vendor_ string to be:

GenuineIntel

which is 12 characters long... plus an unsupplied null termination byte.  Hence
that string is not supplied null terminated earlier, and probably this string is
not terminated here either.

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc#newcode99
base/cpu.cc:99: memcpy(cpu_string, &cpu_info[1], 3*sizeof(int));
Please also use sizeof(cpu_info[1)) instead of sizeof(int).

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc#newcode127
base/cpu.cc:127: for (int parameter = 0x80000002; parameter <= parameter_end;
can you also condition this with:
cpu_string_ptr < &cpu_string[sizeof(cpu_string)]

It makes it much clearer to the reader that you're not overrunning your buffer.

http://codereview.chromium.org/11009011/diff/27001/base/cpu.cc#newcode133
base/cpu.cc:133: cpu_brand_.assign(cpu_string, sizeof(cpu_string));
Instead of sizeof(cpu_string), please use 
cpu_string_ptr - cpu_string
So that you include exactly what you wrote, and nothing more.

[Mark Mentovai, 2012-10-04 18:00:45]

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/26001/base/cpu.cc#newcode135
base/cpu.cc:135: cpu_brand_ = cpu_brand;
jar wrote:
> I couldn't find documentation that suggests the string comes null terminated. 
> The sample documentation provides sample code that does the null termination,
> and worse yet, the example given shows the CPU_vendor_ string to be:
> 
> GenuineIntel
> 
> which is 12 characters long... plus an unsupplied null termination byte. 
Hence
> that string is not supplied null terminated earlier, and probably this string
is
> not terminated here either.

The string in ebx-edx-ecx from cpuid 0 is not NUL-terminated, either in practice
or in documentation. As you say, it’s 12 characters long and the three 32-bit
registers only have room for 12 characters, so there’s no way for it to be
NUL-terminated.

The “brand string” returned by cpuid 0x80000002-0x80000004 is documented as
being NUL-terminated. Ref. Intel64 and IA-32 Architecture Software Developer’s
Manual, vol. 2A (Intel 253666,
http://download.intel.com/products/processor/manual/253666.pdf), August 2012,
vol. 2A page 3-168, CPUID/Methods for Returning Branding Information/How Brand
Stings Work. Ref. also AMD CPUID Specification (AMD 25481,
http://support.amd.com/us/Embedded_TechDocs/25481.pdf), September 2010, page 22,
2 CPUID Function Specification/CPUID Fn8000_000[4:2]_E[D,C,B,A]X Processor Name
String Identifier.

I still wouldn’t rely on that because we’re putting our buffer overflow (or not)
into the control of some external data or our ability to properly call cpuid and
memcpy in this loop. I’d always be precise with the lengths, which is what the
latest patch set does.

[chunyang.dai, 2012-10-04 18:20:47]

In the latest patch, I synchronized the code to resolve one conflict and
modified the cpu.cc according to Jar's comments.

Jar, Could you help to review it again?
thanks

[Mark Mentovai, 2012-10-04 18:24:56]

http://codereview.chromium.org/11009011/diff/29008/base/cpu.cc
File base/cpu.cc (right):

http://codereview.chromium.org/11009011/diff/29008/base/cpu.cc#newcode7
base/cpu.cc:7: #include <algorithm>
Our style guide says that C++ system headers follow C system headers. The
sections are sorted alphabetically. In Chrome, we put a blank link between the
sections.

[commit-bot: I haz the power, 2012-10-04 18:52:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chunyang.dai@intel.com/11009011/37009

[jar (doing other things), 2012-10-04 19:49:58]

Mark is really doing this review... so you can act on his L G T M (above). 
Thanks for making the changes I asked about!


On 2012/10/04 18:20:47, chunyang.dai wrote:
> In the latest patch, I synchronized the code to resolve one conflict and
> modified the cpu.cc according to Jar's comments.
> 
> Jar, Could you help to review it again?
> thanks

[Mark Mentovai, 2012-10-04 20:01:19]

LGTM at patch set 13 for base OWNERS review and approval.

[commit-bot: I haz the power, 2012-10-04 21:05:25]

Change committed as 160226