nasm-devel — NASM development work

Re: [Nasm-devel] [Nasm-bugs] [ nasm-Feature Requests-3370684 ] Make insns.dat more readable (fix attached)

[H. P. A. <hp...@zy...>]

On 07/20/2011 12:19 PM, anonymous coward wrote:
>>>> You can verify that the new insns.dat and insns.pl produce
>>>> byte-identical output to the old insns.dat and insns.pl [...]
>>> if the output is identical, I don't see any reason *not* to merge it
>> I merged the patch.
> 
> The commit only shows insns.dat and insns.pl but no
> testcases -- did that old-vs-new testing happen at all?
> 
> Would sync'ing a tree to before and after this change,
> and then diff'ing the 5 generated insns?.? files suffice?
> 

Yes, that would suffice.  It would be sufficient (but not necessary) to
prove that nothing changed.

	-hpa

Re: [Nasm-devel] [Nasm-bugs] [ nasm-Feature Requests-3370684 ] Make insns.dat more readable (fix attached)

[anonymous c. <nas...@us...>]

>>> You can verify that the new insns.dat and insns.pl produce
>>> byte-identical output to the old insns.dat and insns.pl [...]
>> if the output is identical, I don't see any reason *not* to merge it
> I merged the patch.

The commit only shows insns.dat and insns.pl but no
testcases -- did that old-vs-new testing happen at all?

Would sync'ing a tree to before and after this change,
and then diff'ing the 5 generated insns?.? files suffice?

Re: [Nasm-devel] Switch(size) in assemble.c

[Cyrill G. <gor...@gm...>]

On Tue, Jul 19, 2011 at 07:42:17PM -0700, anonymous coward wrote:
> Could you please refrain from sending one
> full copy after another? Learn how to make
> a branch in git, apply your changes to that
> branch, and lobby others to get your work
> pulled into the main tree from there.
> 
> They invented revision control for a reason.
> 
> Use it!
> 

Yup, the windows version of git could be found here

	http://code.google.com/p/msysgit/

	Cyrill

Re: [Nasm-devel] 'assemble.c ' some changes

[Cyrill G. <gor...@gm...>]

On Tue, Jul 19, 2011 at 03:24:13PM -0700, H. Peter Anvin wrote:
> 
> I'm not really keen on making EMIT_REX() a function unless we can
> profile it and show it doesn't hurt performance...
> 
> 	-hpa

Good point! Mahmoud, lets drop this idea for now.

	Cyrill

Re: [Nasm-devel] Switch(size) in assemble.c

[anonymous c. <nas...@us...>]

Could you please refrain from sending one
full copy after another? Learn how to make
a branch in git, apply your changes to that
branch, and lobby others to get your work
pulled into the main tree from there.

They invented revision control for a reason.

Use it!

Re: [Nasm-devel] 'assemble.c ' some changes

[anonymous c. <nas...@us...>]

>> We really have to find a name for the 64 bytes (case 64; return "";) thing
> Where do we have a 64-byte case?

In practice, at this point in time, nowhere.

Technically Larrabee has 512-bit vector units, but Intel never
published sufficient information to enable tool chain support.

Some day AVX could get widened to 512 bits -- at that point
a choice can be made. (If XMM and YMM have set a tend, it
would seem that ZMM might be next, leading to e.g. zword.)

[Nasm-devel] Switch(size) in assemble.c

[Mahmoud J. <mj...@ho...>]

I have edited switch(size) in assemble.c to give an error in case of a 
size larger than 64.

Sorry for the spam that I have caused, it was a glitch with my email 
client, I am very sorry.

[Nasm-devel] assemble.c size control

[Mahmoud J. <mj...@ho...>]

239,242d238
<     case 65:
< 	case 1024:
<         return "size larger than expected";
< 		sizelargerthan65 = 1; //is set to 1 if switch(size) was larger than 64
250d245
< if ((sizelargerthan65 != 1)){
253,256d247
< 	}
< if ((sizelargerthan65 == 1)){
<             error(ERR_NONFATAL, "error: size larger than expected");
< 	}

[Nasm-devel] Assemble.c size control

[Mahmoud J. <mj...@ho...>]

239,242d238
<     case 65:
< 	case 1024:
<         return "size larger than expected";
< 		sizelargerthan65 = 1; //is set to 1 if switch(size) was larger than 64
250d245
< if ((sizelargerthan65 != 1)){
253,256d247
< 	}
< if ((sizelargerthan65 == 1)){
<             error(ERR_NONFATAL, "error: size larger than expected");
< 	}

Re: [Nasm-devel] 'assemble.c ' some changes

[H. P. A. <hp...@zy...>]

On 07/19/2011 03:07 PM, Cyrill Gorcunov wrote:
> 
> Mahmoud, even if you're sending the whole assemble.c instead of
> patch (or whatever) it must be compilable at least.
> 
> The essential snippeted which has been changed is
> 
> /*EMIT_REX takes no parameters*/
> void EMIT_REX(){ /*If this doesn't work try another type or leave it as a
> define*/                                                         
>     if (!(ins->rex & (REX_D|REX_V)) && (ins->rex & REX_REAL) && (bits == 64)) { 
>         ins->rex = (ins->rex & REX_REAL)|REX_P;                                 
>         out(offset, segment, &ins->rex, OUT_RAWDATA, 1, NO_SEG, NO_SEG);        
>         ins->rex = 0;                                                           
>         offset += 1;                                                            
>     }
> }

I'm not really keen on making EMIT_REX() a function unless we can
profile it and show it doesn't hurt performance...

	-hpa

[Nasm-devel] "assemble.c" function instead of define, patch.

[Mahmoud J. <mj...@ho...>]

Why not using Function instead of #define, Take a look at the patch.

Sorry Cyrill, but I haven't noticed that emit_rex uses parameters in the  
previous code that I posted :S.

Here is a patch so you would be happy ;)

Best,
Mahmoud Jaoune

Re: [Nasm-devel] 'assemble.c ' some changes

[Cyrill G. <gor...@gm...>]

On Tue, Jul 19, 2011 at 11:57:34PM +0000, Mahmoud Jaoune wrote:
> I have changed the #define EMIT_REX() in assemble.c file to a real
> function, I have used 2 ways, and one of them is a comment beneath
> the first one. You may have to change anything that depends on
> normal defining.
> EMIT_REX() takes no parameters.
> 
> Please support me by changing any mistakes I have done ;).
> 
> We really have to find a name for the 64 bytes (case 64; return "";)
> thing :P
> 
> I like sending the whole source file without using patches or
> merging or anything similar, is that OK?
> 
> Best,
> Mahmoud Jaoune

Mahmoud, even if you're sending the whole assemble.c instead of
patch (or whatever) it must be compilable at least.

The essential snippeted which has been changed is

/*EMIT_REX takes no parameters*/
void EMIT_REX(){ /*If this doesn't work try another type or leave it as a
define*/                                                         
    if (!(ins->rex & (REX_D|REX_V)) && (ins->rex & REX_REAL) && (bits == 64)) { 
        ins->rex = (ins->rex & REX_REAL)|REX_P;                                 
        out(offset, segment, &ins->rex, OUT_RAWDATA, 1, NO_SEG, NO_SEG);        
        ins->rex = 0;                                                           
        offset += 1;                                                            
    }
}
/*Or we could use but doesn't differ much
static void EMIT_REX(){ If this doesn't work try another type or leave it as a
define                                                         
    if (!(ins->rex & (REX_D|REX_V)) && (ins->rex & REX_REAL) && (bits == 64)) { 
        ins->rex = (ins->rex & REX_REAL)|REX_P;                                 
        out(offset, segment, &ins->rex, OUT_RAWDATA, 1, NO_SEG, NO_SEG);        
        ins->rex = 0;                                                           
        offset += 1;                                                            
    }
}
*/

Note while being macro it touchs local vars now it tries to access globals,
this is not good at all.

It should be something like

static void emit_rex(insn *ins, int bits, int32_t segment, int64_t *offset)
{
	if (!(ins->rex & (REX_D|REX_V)) && (ins->rex & REX_REAL) && (bits == 64)) {
		ins->rex = (ins->rex & REX_REAL) | REX_P;
		out(*offset, segment, &ins->rex, OUT_RAWDATA, 1, NO_SEG, NO_SEG);
		ins->rex = 0;
		*offset += 1;
	}
}

and probably with 'if' test even more simplified. The idea is to make code
simplier and escape macros if they work like a functions (which allow us to
catch problems in a type of arguments passed).

I think so ;)

	Cyrill

Re: [Nasm-devel] 'assemble.c ' some changes

[Cyrill G. <gor...@gm...>]

On Tue, Jul 19, 2011 at 02:04:14PM -0700, H. Peter Anvin wrote:
> On 07/19/2011 04:57 PM, Mahmoud Jaoune wrote:
> > 
> > I like sending the whole source file without using patches or merging or  
> > anything similar, is that OK?
> > 
> 
> Unified patches are *much* better.
> 
> 	-hpa
> 

Just to make clear by "unified" Peter means this

http://en.wikipedia.org/wiki/Diff#Unified_format

iirc WinMerge can do it.

	Cyrill

[Nasm-devel] request for review and comments

[Cyrill G. <gor...@gm...>]

Hi guys, just pushed out a final pile of cra^W AVX2
instructions, didn't look yet into rest of TBM and
BMI, hope to do on the week.

Still the templates must be re-checked carefully,
so this mail is a call for cooperation ;)

	Cyrill

Re: [Nasm-devel] 'assemble.c ' some changes

[H. P. A. <hp...@zy...>]

On 07/19/2011 04:57 PM, Mahmoud Jaoune wrote:
> 
> I like sending the whole source file without using patches or merging or  
> anything similar, is that OK?
> 

Unified patches are *much* better.

	-hpa

Re: [Nasm-devel] 'assemble.c ' some changes

[H. P. A. <hp...@zy...>]

On 07/19/2011 04:57 PM, Mahmoud Jaoune wrote:
> 
> We really have to find a name for the 64 bytes (case 64; return "";) thing  

Where do we have a 64-byte case?

	-hpa

[Nasm-devel] 'assemble.c ' some changes

[Mahmoud J. <mj...@ho...>]

I have changed the #define EMIT_REX() in assemble.c file to a real  
function, I have used 2 ways, and one of them is a comment beneath the  
first one. You may have to change anything that depends on normal defining.
EMIT_REX() takes no parameters.

Please support me by changing any mistakes I have done ;).

We really have to find a name for the 64 bytes (case 64; return "";) thing  
:P

I like sending the whole source file without using patches or merging or  
anything similar, is that OK?

Best,
Mahmoud Jaoune

Re: [Nasm-devel] assemble.c Minor changes

[H. P. A. <hp...@zy...>]

On 07/19/2011 06:31 AM, Mahmoud Jaoune wrote:
> Some minor changes made to "assemble.c" file, please take a look in the  
> attachments.
> 
> Yours,
> Mahmoud Jaoune

Could you please describe:

1. what is the base of your changes;
2. what is the purpose of your changes?

	-hpa

Re: [Nasm-devel] assemble.c Minor changes

[Cyrill G. <gor...@gm...>]

On Tue, Jul 19, 2011 at 01:31:33PM +0000, Mahmoud Jaoune wrote:
> Some minor changes made to "assemble.c" file, please take a look in
> the attachments.
> 
> Yours,
> Mahmoud Jaoune

Hi Mahmoud,

I managed to get some time for review
(I've applied your code on top of current assemble.c to
 figure out the differences)

diff --git a/assemble.c b/assemble.c
index 790bb12..aff7f05 100644
--- a/assemble.c
+++ b/assemble.c
...
+ * \160..\163    - this instruction uses DREX rather than REX, with the
+ *                 OC0 field set to 0, and the dest field taken from
+ *                 operand 0..3.
+ * \164..\167    - this instruction uses DREX rather than REX, with the
+ *                 OC0 field set to 1, and the dest field taken from
+ *                 operand 0..3.
+ * \171          - placement of DREX suffix in the absence of an EA

DREX has beed deprecated by commit

	fc561203fde370a5ab9db2d089053de51f8a5e04

	    Remove support for DREX encoding

	    The DREX encoding never hit production silicon, and has been replaced
	    by VEX/XOP encoding, so remove support for it.

	    Signed-off-by: H. Peter Anvin <hp...@li...>

...

 static int has_prefix(insn * ins, enum prefix_pos pos, enum prefixes prefix)
 {
     return ins->prefixes[pos] == prefix;
@@ -236,8 +239,10 @@ static const char *size_name(int size)
         return "oword";
     case 32:
         return "yword";
+    case 64:
+        return "64 Bytes";		

I guess we need a prefix here rather than 64 bytes, though I forget
which prefix would be fine ;) 

     default:
-        return "???";
+        return "Size Unknown";
     }
 } 
@@ -450,7 +455,7 @@ int64_t assemble(int32_t segment, int64_t offset, int bits, uint32_t cp,
                          * it.
                          */
                         error(ERR_NONFATAL,
-                              "`incbin': unexpected EOF while"
+                              "`incbin': unexpected 'End Of File' while"
                               " reading file `%s'", fname);
                         t = 0;  /* Try to exit cleanly */
                         break;

EOF is well known abbreviation ;)

...

         c = *codes++;
@@ -1227,11 +1244,15 @@ static void gencode(int32_t segment, int64_t offset, int bits,
         case 02:
         case 03:
         case 04:
+		#ifdef EMIT_REX()
             EMIT_REX();
             out(offset, segment, codes, OUT_RAWDATA, c, NO_SEG, NO_SEG);
             codes += c;
             offset += c;
             break;
+		#else
+        printf ("NASM internal execution Error!");
+        #endif

This is not needed as well ;) If EMIT_REX is not defined we will
get compilation error, which is pretty fine (I think I would rather
switch to helper function here at all instead of this macro).

So Mahmoud, I think this patch can't be merged at moment. Still
it's *quite* good that you desided to start nasm hacking! Please
don't hesitate to send patches.

(btw you might be insterested in using say WinMerge which produce
 pretty good patches)

	Cyrill

Re: [Nasm-devel] assemble.c Minor changes

[Cyrill G. <gor...@gm...>]

On Tue, Jul 19, 2011 at 01:44:37PM +0000, Mahmoud Jaoune wrote:
> 
> I used Notepad++ :S
> You may find some unexpected changes because I am still new to NASM Devel
> and I only made minor changes for that.
> 
> Best,
> Mahmoud Jaoune
> 

No problem ;)

	Cyrill

Re: [Nasm-devel] assemble.c Minor changes

[Mahmoud J. <mj...@ho...>]

> On Tue, Jul 19, 2011 at 01:31:33PM +0000, Mahmoud Jaoune wrote:
>> Some minor changes made to "assemble.c" file, please take a look in
>> the attachments.
>>
>> Yours,
>> Mahmoud Jaoune
>
> Hi Mahmoud, I'll try to find time tonight for review, thanks!
> Meanwhile please check next time that you don't change end of
> strings from LF to CRLF. I guess most modern editors can
> do it automatically? At least last time I tried Notepad++
> it handled LFs pretty well ;)
>
> 	Cyrill

I used Notepad++ :S
You may find some unexpected changes because I am still new to NASM Devel
and I only made minor changes for that.

Best,
Mahmoud Jaoune

Re: [Nasm-devel] assemble.c Minor changes

[Cyrill G. <gor...@gm...>]

On Tue, Jul 19, 2011 at 01:31:33PM +0000, Mahmoud Jaoune wrote:
> Some minor changes made to "assemble.c" file, please take a look in
> the attachments.
> 
> Yours,
> Mahmoud Jaoune

Hi Mahmoud, I'll try to find time tonight for review, thanks!
Meanwhile please check next time that you don't change end of
strings from LF to CRLF. I guess most modern editors can
do it automatically? At least last time I tried Notepad++
it handled LFs pretty well ;)

	Cyrill

[Nasm-devel] assemble.c Minor changes

[Mahmoud J. <mj...@ho...>]

Some minor changes made to "assemble.c" file, please take a look in the  
attachments.

Yours,
Mahmoud Jaoune

Re: [Nasm-devel] [nasm:master] A few more AVX2 spec instructions

[anonymous c. <nas...@us...>]

> A few more AVX2 spec instructions

Attached find the binutils/gas testcases
converted to NASM code.

Re: [Nasm-devel] Website Refresh

[Cyrill G. <gor...@gm...>]

On Thu, Jul 07, 2011 at 09:03:46AM -0700, H. Peter Anvin wrote:
> On 07/07/2011 06:29 AM, Keith Kanios wrote:
> > 
> > Yeah, Fossil is by the same person/group behind SQLite. Fossil is also a 
> > (D)VCS/repository system, but we wouldn't need to use that if we are 
> > sticking with GIT. However, here is some basic info for GIT vs Fossil: 
> > http://www.sqlite.org/debug1/doc/trunk/www/fossil-v-git.wiki
> > 
> 
> I find that link to be full of "truths masquerading as lies"; in other
> words to be (at least mostly) correct but presented in such a way to
> deliberately create the wrong overall perception.
> 
> 	-hpa
>

ROFL ;)
 
	Cyrill