-
-
Notifications
You must be signed in to change notification settings - Fork 594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minor memory-safety fixes (Valgrind-identified problems, debug bounds checks) #5068
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2310,6 +2310,9 @@ void MachObj::addrel(int seg, targ_size_t offset, symbol *targsym, | |
unsigned targseg, int rtype, int val) | ||
{ | ||
Relocation rel; | ||
#ifdef DEBUG | ||
memset(&rel, 0, sizeof(rel)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be Relocation rel = {}; |
||
#endif | ||
rel.offset = offset; | ||
rel.targsym = targsym; | ||
rel.targseg = targseg; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -796,8 +796,25 @@ extern (C++) Expression Expression_optimize(Expression e, int result, bool keepL | |
return; | ||
} | ||
// If e2 *could* have been an integer, make it one. | ||
if (e.e2.op == TOKfloat64 && (e.e2.toReal() == cast(sinteger_t)e.e2.toReal())) | ||
e.e2 = new IntegerExp(e.loc, e.e2.toInteger(), Type.tint64); | ||
if (e.e2.op == TOKfloat64) | ||
{ | ||
version (all) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some way we could make this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ping on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a specific suggestion I can add? In principle, this can only be safely removed once DMD is no longer compilable with DMD 2.068 or earlier, which probably won't be for a while. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean that is should either be checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, added a comment. But I should note that these patches are not comprehensive, and are only what I needed to fix in order to be able to debug issue 15019. I would like to run the DMD test suite under Valgrind some day (I know it'll take days to complete, probably), but not now. Also, IMHO, this seems like quite a bit of bureaucracy for such a simple change (extracting a few expressions to variables). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like adding dead code, and I've put quite a lot of effort into removing it. I have no problem with the change, and I've done similar things myself, my problem is with the dead code Walter asked for. If we're going to see dozens of similar changes then it might be better to look at other options (like patching the binaries to replace the prefix with a nop). |
||
{ | ||
// Work around redundant REX.W prefix breaking Valgrind | ||
// when built with affected versions of DMD. | ||
// https://issues.dlang.org/show_bug.cgi?id=14952 | ||
// This can be removed once compiling with DMD 2.068 or | ||
// older is no longer supported. | ||
d_float80 r = e.e2.toReal(); | ||
if (r == cast(sinteger_t)r) | ||
e.e2 = new IntegerExp(e.loc, e.e2.toInteger(), Type.tint64); | ||
} | ||
else | ||
{ | ||
if (e.e2.toReal() == cast(sinteger_t)e.e2.toReal()) | ||
e.e2 = new IntegerExp(e.loc, e.e2.toInteger(), Type.tint64); | ||
} | ||
} | ||
if (e.e1.isConst() == 1 && e.e2.isConst() == 1) | ||
{ | ||
Expression ex = Pow(e.type, e.e1, e.e2).copy(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the fields of rel are assigned values here. Adding a memset() is completely redundant, and makes it slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relocation
has padding bytes, which would otherwise be filled with garbage and written to disk. Garbage bytes in output make it difficult to compare created objects and prohibit bit-identical builds. This is definitely not "completely redundant" and Valgrind is right to complain about it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this struct is ever written to disk. It's for use by machobj.c only. Your point about holes is valid. But because this is a performance critical issue for this type, I suggest reorganizing the Relocation struct so that there are no holes. I.e.:
should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, what's this line doing at the bottom of this very function then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SDrel is being used as a variable length array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK, I see what's going on. I added validation to
Outbuffer
so that I can see in the stack trace when garbage is written toOutbuffer
, not whenOutbuffer
writes said garbage to a file. This results in false positives whenOutbuffer
is not being used to buffer things later written to disk. You were right, sorry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still reasonable to memset in debug mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The right fix is to reorganize the type as I suggested.