Re: RFR: JDK-8151656: Minor tweaks to old Hotspot build to ease comparison with new

[Christian Thalinger]

> On Mar 15, 2016, at 10:51 PM, Erik Joelsson  wrote:
> 
> The __FILE__ macro is built into the compiler and evaluates to the file name 
> of the current source file. It's intended use is for debug messages. In the 
> rest of the JDK we eliminated its use completely by providing a macro called 
> THIS_FILE that evaluates to the basename.ext of the source file being 
> compiled. In Hotspot it's still used and it's used in header files, which 
> makes the THIS_FILE solution invalid.
> 
> For most of our compilers, the format of the path is based on how the file 
> was presented on the command line. This matters to us since the old Hotspot 
> build uses relative paths to the generated source and header files and the 
> new build uses absolute paths for all source files. Also, the directory 
> structure in the output directory differ. This means that for certain object 
> files, the constant strings resulting from the use of __FILE__ differ, both 
> in contents and length. The difference in length causes alignment differences 
> in the objects and the resulting libraries.

This is actually a very good point.  Alignment differences can cause 
performance differences.  Sometimes hard to track down and almost impossible to 
reproduce.

> 
> When creating the new build, we are trying very hard to make sure we are 
> still building the same thing. One way to verify this is to compare various 
> aspects of the built binaries. When possible, this is far easier than running 
> a very large amount of tests and it gives us a large amount of confidence in 
> our changes. The alignment differences caused by different lengths of 
> constant strings severely limits the amount of clean comparisons we can do.
> 
> The particular fix here changes how we generate the files from the ADLC tool. 
> The tool already generates #line directives which helps the compiler figure 
> out which actual source file and line parts of the generated files came from. 
> Basically overriding what __FILE__ and __LINE__ will evaluate to. Some of 
> these lines need a bit of post processing, and for that, the build already 
> uses awk to rewrite them. The current awk script looks for lines like this, 
> which the ADLC tool prints when it doesn't know the correct source file:
> 
> #line 99
> 
> and rewrites it to
> 
> #line   file>
> 
> I changed this to instead rewrite to:
> 
> #line  

You removed the +1.  Why was it used in the first place?

> 
> I also added an initial #line first in each file.
> 
> #line 1 
> 
> With these changes, we can do very clean comparisons on Linux, Solaris and 
> Macosx. This helps the new hotspot build immensely, but will also be good in 
> the future as we have a very convenient way of verifying build changes that 
> shouldn't affect the built output.

Now this all makes sense.  Thanks.  The change looks good to me.

> 
> /Erik
> 
> On 2016-03-15 22:02, Christian Thalinger wrote:
>>> On Mar 15, 2016, at 12:06 AM, Erik Joelsson  
>>> wrote:
>>> 
>>> Any chance I could get a second review on this? Preferably from the hotspot 
>>> team.
>> It’s not quite obvious to me what the __FILE__ change is.  Maybe an example 
>> would help.
>> 
>>> /Erik
>>> 
>>> On 2016-03-11 18:16, Erik Joelsson wrote:
 In preparation for the new Hotspot build, there are a few changes to the 
 old build that needs to happen first. These changes should be harmless, 
 but do impact the generated binaries some. These changes are:
 
 * Standardizing sort order for objects on link command line on Windows to 
 a locale independent order.
 * Working around compare differences caused by the __FILE__ macro in 
 certain generated files by overriding the value of __FILE__ in those files.
 
 By making these changes first and separate from introducing the new build 
 we reduce the potential impact of when we do introduce the new build.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8151656
 Webrev: http://cr.openjdk.java.net/~erikj/8151656/index.html
 
 /Erik
> 

Re: RFR: JDK-8151656: Minor tweaks to old Hotspot build to ease comparison with new

[Erik Joelsson]

On 2016-03-16 19:17, Christian Thalinger wrote:

On Mar 15, 2016, at 10:51 PM, Erik Joelsson  wrote:

The __FILE__ macro is built into the compiler and evaluates to the file name of 
the current source file. It's intended use is for debug messages. In the rest 
of the JDK we eliminated its use completely by providing a macro called 
THIS_FILE that evaluates to the basename.ext of the source file being compiled. 
In Hotspot it's still used and it's used in header files, which makes the 
THIS_FILE solution invalid.

For most of our compilers, the format of the path is based on how the file was 
presented on the command line. This matters to us since the old Hotspot build 
uses relative paths to the generated source and header files and the new build 
uses absolute paths for all source files. Also, the directory structure in the 
output directory differ. This means that for certain object files, the constant 
strings resulting from the use of __FILE__ differ, both in contents and length. 
The difference in length causes alignment differences in the objects and the 
resulting libraries.

This is actually a very good point.  Alignment differences can cause 
performance differences.  Sometimes hard to track down and almost impossible to 
reproduce.


When creating the new build, we are trying very hard to make sure we are still 
building the same thing. One way to verify this is to compare various aspects 
of the built binaries. When possible, this is far easier than running a very 
large amount of tests and it gives us a large amount of confidence in our 
changes. The alignment differences caused by different lengths of constant 
strings severely limits the amount of clean comparisons we can do.

The particular fix here changes how we generate the files from the ADLC tool. 
The tool already generates #line directives which helps the compiler figure out 
which actual source file and line parts of the generated files came from. 
Basically overriding what __FILE__ and __LINE__ will evaluate to. Some of these 
lines need a bit of post processing, and for that, the build already uses awk 
to rewrite them. The current awk script looks for lines like this, which the 
ADLC tool prints when it doesn't know the correct source file:

#line 99

and rewrites it to

#line  

I changed this to instead rewrite to:

#line  

You removed the +1.  Why was it used in the first place?
I honestly have no idea. The existing code adds 1, but from what I can 
see, it's incorrect to do so. In my change, since I'm adding a new line 
first in the file, it is correct to add 1 so I left that in the awk 
script. That's what I mean with "actual line number in generated file" 
without +1.



I also added an initial #line first in each file.

#line 1 

With these changes, we can do very clean comparisons on Linux, Solaris and 
Macosx. This helps the new hotspot build immensely, but will also be good in 
the future as we have a very convenient way of verifying build changes that 
shouldn't affect the built output.

Now this all makes sense.  Thanks.  The change looks good to me.

Thanks!

/Erik

/Erik

On 2016-03-15 22:02, Christian Thalinger wrote:

On Mar 15, 2016, at 12:06 AM, Erik Joelsson  wrote:

Any chance I could get a second review on this? Preferably from the hotspot 
team.

It’s not quite obvious to me what the __FILE__ change is.  Maybe an example 
would help.


/Erik

On 2016-03-11 18:16, Erik Joelsson wrote:

In preparation for the new Hotspot build, there are a few changes to the old 
build that needs to happen first. These changes should be harmless, but do 
impact the generated binaries some. These changes are:

* Standardizing sort order for objects on link command line on Windows to a 
locale independent order.
* Working around compare differences caused by the __FILE__ macro in certain 
generated files by overriding the value of __FILE__ in those files.

By making these changes first and separate from introducing the new build we 
reduce the potential impact of when we do introduce the new build.

Bug: https://bugs.openjdk.java.net/browse/JDK-8151656
Webrev: http://cr.openjdk.java.net/~erikj/8151656/index.html

/Erik

Re: RFR: JDK-8151656: Minor tweaks to old Hotspot build to ease comparison with new

[Erik Joelsson]

The __FILE__ macro is built into the compiler and evaluates to the file 
name of the current source file. It's intended use is for debug 
messages. In the rest of the JDK we eliminated its use completely by 
providing a macro called THIS_FILE that evaluates to the basename.ext of 
the source file being compiled. In Hotspot it's still used and it's used 
in header files, which makes the THIS_FILE solution invalid.


For most of our compilers, the format of the path is based on how the 
file was presented on the command line. This matters to us since the old 
Hotspot build uses relative paths to the generated source and header 
files and the new build uses absolute paths for all source files. Also, 
the directory structure in the output directory differ. This means that 
for certain object files, the constant strings resulting from the use of 
__FILE__ differ, both in contents and length. The difference in length 
causes alignment differences in the objects and the resulting libraries.


When creating the new build, we are trying very hard to make sure we are 
still building the same thing. One way to verify this is to compare 
various aspects of the built binaries. When possible, this is far easier 
than running a very large amount of tests and it gives us a large amount 
of confidence in our changes. The alignment differences caused by 
different lengths of constant strings severely limits the amount of 
clean comparisons we can do.


The particular fix here changes how we generate the files from the ADLC 
tool. The tool already generates #line directives which helps the 
compiler figure out which actual source file and line parts of the 
generated files came from. Basically overriding what __FILE__ and 
__LINE__ will evaluate to. Some of these lines need a bit of post 
processing, and for that, the build already uses awk to rewrite them. 
The current awk script looks for lines like this, which the ADLC tool 
prints when it doesn't know the correct source file:


#line 99

and rewrites it to

#line  generated file>


I changed this to instead rewrite to:

#line  file>


I also added an initial #line first in each file.

#line 1 

With these changes, we can do very clean comparisons on Linux, Solaris 
and Macosx. This helps the new hotspot build immensely, but will also be 
good in the future as we have a very convenient way of verifying build 
changes that shouldn't affect the built output.


/Erik

On 2016-03-15 22:02, Christian Thalinger wrote:

On Mar 15, 2016, at 12:06 AM, Erik Joelsson  wrote:

Any chance I could get a second review on this? Preferably from the hotspot 
team.

It’s not quite obvious to me what the __FILE__ change is.  Maybe an example 
would help.


/Erik

On 2016-03-11 18:16, Erik Joelsson wrote:

In preparation for the new Hotspot build, there are a few changes to the old 
build that needs to happen first. These changes should be harmless, but do 
impact the generated binaries some. These changes are:

* Standardizing sort order for objects on link command line on Windows to a 
locale independent order.
* Working around compare differences caused by the __FILE__ macro in certain 
generated files by overriding the value of __FILE__ in those files.

By making these changes first and separate from introducing the new build we 
reduce the potential impact of when we do introduce the new build.

Bug: https://bugs.openjdk.java.net/browse/JDK-8151656
Webrev: http://cr.openjdk.java.net/~erikj/8151656/index.html

/Erik

Re: RFR: JDK-8151656: Minor tweaks to old Hotspot build to ease comparison with new

[Christian Thalinger]

> On Mar 15, 2016, at 12:06 AM, Erik Joelsson  wrote:
> 
> Any chance I could get a second review on this? Preferably from the hotspot 
> team.

It’s not quite obvious to me what the __FILE__ change is.  Maybe an example 
would help.

> 
> /Erik
> 
> On 2016-03-11 18:16, Erik Joelsson wrote:
>> In preparation for the new Hotspot build, there are a few changes to the old 
>> build that needs to happen first. These changes should be harmless, but do 
>> impact the generated binaries some. These changes are:
>> 
>> * Standardizing sort order for objects on link command line on Windows to a 
>> locale independent order.
>> * Working around compare differences caused by the __FILE__ macro in certain 
>> generated files by overriding the value of __FILE__ in those files.
>> 
>> By making these changes first and separate from introducing the new build we 
>> reduce the potential impact of when we do introduce the new build.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151656
>> Webrev: http://cr.openjdk.java.net/~erikj/8151656/index.html
>> 
>> /Erik
> 

Re: RFR: JDK-8151656: Minor tweaks to old Hotspot build to ease comparison with new

[Erik Joelsson]

Any chance I could get a second review on this? Preferably from the 
hotspot team.


/Erik

On 2016-03-11 18:16, Erik Joelsson wrote:
In preparation for the new Hotspot build, there are a few changes to 
the old build that needs to happen first. These changes should be 
harmless, but do impact the generated binaries some. These changes are:


* Standardizing sort order for objects on link command line on Windows 
to a locale independent order.
* Working around compare differences caused by the __FILE__ macro in 
certain generated files by overriding the value of __FILE__ in those 
files.


By making these changes first and separate from introducing the new 
build we reduce the potential impact of when we do introduce the new 
build.


Bug: https://bugs.openjdk.java.net/browse/JDK-8151656
Webrev: http://cr.openjdk.java.net/~erikj/8151656/index.html

/Erik

Re: RFR: JDK-8151656: Minor tweaks to old Hotspot build to ease comparison with new

[Tim Bell]

Erik:

In preparation for the new Hotspot build, there are a few changes to 
the old build that needs to happen first. These changes should be 
harmless, but do impact the generated binaries some. These changes are:


* Standardizing sort order for objects on link command line on Windows 
to a locale independent order.
* Working around compare differences caused by the __FILE__ macro in 
certain generated files by overriding the value of __FILE__ in those 
files.


By making these changes first and separate from introducing the new 
build we reduce the potential impact of when we do introduce the new 
build.


Bug: https://bugs.openjdk.java.net/browse/JDK-8151656
Webrev: http://cr.openjdk.java.net/~erikj/8151656/index.html


OK, looks good to me.

/Tim

RFR: JDK-8151656: Minor tweaks to old Hotspot build to ease comparison with new

[Erik Joelsson]

In preparation for the new Hotspot build, there are a few changes to the 
old build that needs to happen first. These changes should be harmless, 
but do impact the generated binaries some. These changes are:


* Standardizing sort order for objects on link command line on Windows 
to a locale independent order.
* Working around compare differences caused by the __FILE__ macro in 
certain generated files by overriding the value of __FILE__ in those files.


By making these changes first and separate from introducing the new 
build we reduce the potential impact of when we do introduce the new build.


Bug: https://bugs.openjdk.java.net/browse/JDK-8151656
Webrev: http://cr.openjdk.java.net/~erikj/8151656/index.html

/Erik