On 23/01/2014 00:09, Julian Lettner wrote:
Thanks for your comments, Alp.
Regarding the static / global issue, I agree and I will try to take
care of it.
Thanks Julian. To clarify, it's OK if you keep the current simple salt
implementation to begin with as long you're committed to improving it in
tree.
That way the frontend developers can work with you to find the right
design -- having patches out of tree clearly hasn't worked out so let's
see if we can get this unblocked with a more direct approach.
@Stephen, Andrei
What are your opinionson un-seeded / non-deterministic compilation?
I'm fairly convinced that a non-deterministic mode doesn't belong in
clang (or indeed LLVM). We need to preserve invariants that modern build
systems expect and you've already done the hard work to provide a
predictable PRNG which should be superior in every case.
If having the seed on the commandline is a security concern due to
logging, just add a TODO to address that later through other means like
a response file?
PS: Our discussion does not get aggregated here:
http://llvm-reviews.chandlerc.com/D1803
Did I do something wrong? How can we change that?
I don't use Phabricator and its lack of email threading seems to have
added to the confusion surrounding this patchset.
It seems reasonable at this point to cross-post a new email thread to
both llvm-commits and cfe-commits containing both patches attached
rebased to ToT so we can get a clear view of where things stand are and
set a direction where development continues in-tree.
Thanks for working on this!
Alp.
On Wed, Jan 22, 2014 at 3:50 PM, Alp Toker <[email protected]
<mailto:[email protected]>> wrote:
Stephen,
I've looked a bit closer at the clang patch.
I don't understand why this is global:
llvm::RandomNumberGenerator::SetSalt(SaltString);
LLVM and clang have a strict library design so this would be
unreliable for anything other than the simplest single-threaded
sequential use modes.
We're getting close to fixing the last remaining statics so it
doesn't seem right to introduce a new one.
Alp.
On 22/01/2014 23:39, Alp Toker wrote:
On 22/01/2014 23:17, Stephen Crane wrote:
Here's the patch for LLVM:
http://llvm-reviews.chandlerc.com/D1802 We ended up basing
the RNG on the already integrated implementation of MD5,
to avoid any external dependencies. We are really just
waiting on review of the LLVM patch now that Julian has
modified a few things to take care of a performance concern.
That sounds good.
David Majnemer has already done preliminary review of the
clang patch and it looks sane to me.
It will additionally need user documentation explaining the
purpose of the feature and noting that stability is not
guaranteed between different revisions of the compiler, even
with the same seed.
It's my opinion that un-seeded / non-deterministic compilation
shouldn't be supported at all. If that isn't the case already
would it be reasonable change for you to accommodate?
Apart from that, just blocked on the LLVM changes.
Alp.
- stephen
On Wed, Jan 22, 2014 at 3:00 PM, Alp Toker <[email protected]
<mailto:[email protected]> <mailto:[email protected]
<mailto:[email protected]>>> wrote:
The clang side looks fine, but there's very little
context as to
what's going on here so not possible to review it just
like that.
The patch rebases to clang ToT fine but doesn't build
due to
missing RNG facilities in LLVM -- could you give a
refresher of
the status of that with a link? It's been long enough
that not
everyone remembers the discussion.
The last I remember of the discussion was that linking
to OpenSSL
can be painful, and it doesn't feel right as a
dependency. What
are the other options for pseudo RNG and could we have
a simpler
scheme?
That'll help get things moving.
Alp.
On 22/01/2014 21:48, Julian Lettner wrote:
Is there anything stopping this from going forward?
http://llvm-reviews.chandlerc.com/D1803
_______________________________________________
cfe-commits mailing list
[email protected] <mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-- http://www.nuanti.com
the browser experts
--
http://www.nuanti.com
the browser experts
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
