Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Minor memory-safety fixes (Valgrind-identified problems, debug bounds checks) #5068

Merged
merged 3 commits into from
Sep 22, 2015
Merged

Minor memory-safety fixes (Valgrind-identified problems, debug bounds checks) #5068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bc2030
Work around Valgrind choking on redundant REX.W prefix
CyberShadow Sep 12, 2015
8735fb8
Fix potential memory issues identified by Valgrind
CyberShadow Sep 12, 2015
d7091d5
hdrgen: Use OutBuffer.extractData()
CyberShadow Sep 13, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/backend/machobj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.:

struct Relocation
{
    symbol *funcsym;
    symbol *targsym;
    unsigned offset;
    unsigned targseg;
    unsigned rtype;
    int val;
}

should do it.

Copy link
Member Author

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.

Um, what's this line doing at the bottom of this very function then?

 pseg->SDrel->write(&rel, sizeof(rel));

Copy link
Member

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.

Copy link
Member Author

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 to Outbuffer, not when Outbuffer writes said garbage to a file. This results in false positives when Outbuffer is not being used to buffer things later written to disk. You were right, sorry.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down
16 changes: 15 additions & 1 deletion src/constfold.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,21 @@ extern (C++) UnionExp Div(Type type, Expression e1, Expression e2)
{
if (e1.type.isreal())
{
emplaceExp!(RealExp)(&ue, loc, e1.toReal() / e2.toReal(), type);
version (all)
{
// 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 r1 = e1.toReal();
d_float80 r2 = e2.toReal();
emplaceExp!(RealExp)(&ue, loc, r1 / r2, type);
}
else
{
emplaceExp!(RealExp)(&ue, loc, e1.toReal() / e2.toReal(), type);
}
return ue;
}
r = e2.toReal();
Expand Down
2 changes: 1 addition & 1 deletion src/dmangle.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ public:
scope Mangler v = new Mangler(&buf2);
v.paramsToDecoBuffer(t.arguments);
int len = cast(int)buf2.offset;
buf.printf("%d%.*s", len, len, buf2.extractData());
buf.printf("%d%.*s", len, len, buf2.extractString());
}

void visit(TypeNull t)
Expand Down
2 changes: 1 addition & 1 deletion src/hdrgen.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ extern (C++) void genhdrfile(Module m)
toCBuffer(m, &buf, &hgs);
// Transfer image to file
m.hdrfile.setbuffer(buf.data, buf.offset);
buf.data = null;
buf.extractData();
ensurePathToNameExists(Loc(), m.hdrfile.toChars());
writeFile(m.loc, m.hdrfile);
}
Expand Down
21 changes: 19 additions & 2 deletions src/optimize.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way we could make this static if (version < firstFixedVersion) so that it's immediately clear when it can be removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping on this?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that is should either be checking __VERSION__ instead of using version(all), or at least have a comment saying that it can be removed once 2.068 is no longer supported. The bugzilla link and description are great but having the version number explicitly there as well makes it obvious.

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

Copy link
Member

Choose a reason for hiding this comment

The 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();
Expand Down