Re: CVS commit: src

2013-12-02 Thread Alexander Nasonov
Lourival Pereira Vieira Neto wrote:
 Module Name:  src
 Committed By: lneto
 Date: Mon Dec  2 06:07:22 UTC 2013
 
 Modified Files:
   src/external/mit/lua/dist/src: luaconf.h
   src/sys/modules/lua: Makefile
 Removed Files:
   src/sys/modules/lua: luaconf.h
 
 Log Message:
 merged luaconf.h of kernel and userspace Lua


1. This doesn't look right:

#define luai_numpow(a,b)luai_nummul(a,b)



2. If intmax_t is a greater type than int64_t, the below doesn't
handle overflow:
#define lua_str2number(s,p)((int64_t) strtoimax((s), (p), 10))

I checked Lua code and there is no overflow check. However, I noticed
that they detect 0x strings and convert them using strtoul. It should
be changed to stroumax. This is in function luaO_str2d in src/lobject.c.

3. Item 1 was in my earlier review of luaconf.h in sys/modules and I see
at least one minor item not covered by your new change. Can you please 
go over my review and make sure all covered?

Thanks,
Alex


Re: CVS commit: src

2013-12-02 Thread Lourival Vieira Neto
Hi Alan,

 Log Message:
 changed lua_Number to int64_t

 This was actually two logical changes:

 * move strtoimax from the userland-only part of libc to the common part 
 that's also
 usable by the kernel.

 * change lua_Number to int64_t for the kernel Lua implementation.

 I would have preferred to see two separate commits.  Even with only
 one commit, the log message should have described both changes.

Sorry for that. I posted the patch on tech-kern, and I thought that it
was OK for single committing. Next time I'll separate it better.

Regards,
-- 
Lourival Vieira Neto


Re: CVS commit: src/common/lib/libc/stdlib

2013-12-02 Thread Lourival Vieira Neto
 Module Name:src
 Committed By:   joerg
 Date:   Mon Dec  2 12:20:44 UTC 2013

 Modified Files:
src/common/lib/libc/stdlib: strtoimax.c

 Log Message:
 Fix aliases.

Sorry about that.. my bad!

Regards,
-- 
Lourival Vieira Neto


Re: CVS commit: src

2013-12-02 Thread Alexander Nasonov
Lourival Vieira Neto wrote:
 Yes, it isn't. But, please note, I didn't change that now. I just
 merged it in one single file. Though I think we need implement integer
 exponentiation, I think that is not a priority. IMO, it is a TODO.

Well, I assume that any raised issues should be resolved before moving
broken stuff around. See the link to my review below.

If you wanted to keep this item in TODO list, you'd better picked
lua_error rather than lua_mul ;-)


  2. If intmax_t is a greater type than int64_t, the below doesn't
  handle overflow:
  #define lua_str2number(s,p)((int64_t) strtoimax((s), (p), 10))
 
 I'm considering two approaches:
 
 1) creating an auxiliary function based on strtoimax():
...
 2) creating an auxiliary function based on strtol template

I think you created some problems for yourself by using int64_t. I was
going to tell you that Lua 5.3 uses long long for 64bit integers but I
saw your question on lua-l about it.

I didn't mind using long long in the kernel when we discussed that topic
but some other people convinced you to use explicit width type ;-)

  3. Item 1 was in my earlier review of luaconf.h in sys/modules and I see
  at least one minor item not covered by your new change. Can you please
  go over my review and make sure all covered?
 
 Sorry, I've missed that. Which review? Are you talking about the
 changing lua_Number to int64_t thread?

http://mail-index.netbsd.org/source-changes-d/2013/10/22/msg006172.html

Alex


Re: CVS commit: src

2013-12-02 Thread Lourival Vieira Neto
Hi Alexander,

On Mon, Dec 2, 2013 at 8:38 PM, Alexander Nasonov al...@yandex.ru wrote:
 Lourival Vieira Neto wrote:
 Yes, it isn't. But, please note, I didn't change that now. I just
 merged it in one single file. Though I think we need implement integer
 exponentiation, I think that is not a priority. IMO, it is a TODO.

 Well, I assume that any raised issues should be resolved before moving
 broken stuff around. See the link to my review below.

I wasn't in that thread at that time. However, I'll carefully read it.
Anyway, I think that the missing implementation of luai_numpow()
doesn't break anything. Yes, of course, a*b != pow(a,b); but, it was
left in that way just for convenience. I don't see this as a main
issue anyhow.

 If you wanted to keep this item in TODO list, you'd better picked
 lua_error rather than lua_mul ;-)

Is there an issue with lua_error? Anyway, having two different
luaconf.h was a real issue. I have problems including the wrong header
with a little more complex Lua kmods (such as luadata). In fact, all
Lua kmods were including the wrong luaconf.h file, because of the
order of inclusion (-I's on Makefile). It didn't explode because they
weren't using lua_Number, but I had a real hard time trying to debug
this on luadata, once that compiling a kmod that uses a double type
isn't raising an error on amd64 (don't know why).

  2. If intmax_t is a greater type than int64_t, the below doesn't
  handle overflow:
  #define lua_str2number(s,p)((int64_t) strtoimax((s), (p), 10))

 I'm considering two approaches:

 1) creating an auxiliary function based on strtoimax():
 ...
 2) creating an auxiliary function based on strtol template

 I think you created some problems for yourself by using int64_t. I was
 going to tell you that Lua 5.3 uses long long for 64bit integers but I
 saw your question on lua-l about it.

 I didn't mind using long long in the kernel when we discussed that topic
 but some other people convinced you to use explicit width type ;-)

I don't think I've created a problem to myself. We discussed it on the
list and _we_ come to a conclusion. Note, I don't think that this
overflow is a huge problem. The worst that could happen is to have a
truncation of a greater number instead of 0. Anyway, I think it must
be fixed and that any of the two presented solutions is fine. However,
I also have no problem to step back and use 'long long', if _we_
choose to reconsider that. IMHO, the fact that Lua 5.3 is using 'long
long' is a good argument for that. I do prefer explicit width type,
but my main argument is that 'long long' width could be lesser than 64
bit.

Regards,
-- 
Lourival Vieira Neto


Re: CVS commit: src/lib/libc/compiler_rt

2013-12-02 Thread Matt Thomas

On Dec 2, 2013, at 6:28 PM, Joerg Sonnenberger jo...@netbsd.org wrote:

 Module Name:  src
 Committed By: joerg
 Date: Tue Dec  3 02:28:51 UTC 2013
 
 Modified Files:
   src/lib/libc/compiler_rt: Makefile.inc
 
 Log Message:
 Add ARM (EABI) specific sources. Split off code that requires C11,
 unwind support in libc or overlaps with soft-float in preparation for
 using compiler-rt in the non-clang case.

How will interact with libkern?