Re: CVS commit: src/lib/libc/compiler_rt
On Dec 2, 2013, at 6:28 PM, Joerg Sonnenberger 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?
Re: CVS commit: src
Hi Alexander, On Mon, Dec 2, 2013 at 8:38 PM, Alexander Nasonov 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
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/common/lib/libc/stdlib
> 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
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
Hi Alexander,
> 1. This doesn't look right:
>
> #define luai_numpow(a,b)luai_nummul(a,b)
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.
> 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():
int64_t
strtoint64(const char *nptr, char **endptr, int base)
{
imax_t imax = strtoimax(nptr, endptr, base);
return (int64_t) (imax >= INT64_MIN && imax <= INT64_MAX ? imax : 0);
}
#define lua_str2number(s,p)(strtoint64((s), (p), 10))
2) creating an auxiliary function based on strtol template
(...)
#define _FUNCNAME strtoint64
#define __INT int64_t
#define __INT_MIN INT64_MIN
#define __INT_MAX INT64_MAX
#include "_strtol.h"
(...)
I prefer the first approach, because it uses a std function without
re-implementing it. Waiting to hear your and others considerations.
> 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.
I'll check that.
> 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?
Regards,
--
Lourival Vieira Neto
Re: CVS commit: src
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
