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

CyberShadow · 2015-09-13T00:40:41Z

Currently some debug code to run against the autotester to help debug issue 15019:

https://issues.dlang.org/show_bug.cgi?id=15019

WalterBright · 2015-09-13T00:50:14Z

I thought the REX.W prefix bug was fixed and pulled. If there's another one, please let me know where.

CyberShadow · 2015-09-13T00:55:26Z

I thought the REX.W prefix bug was fixed and pulled.

Yes, thanks. But you need to bootstrap through DMD head to take advantage of the fix. There are cases when you don't want to do that (e.g. Digger would need to compile DMD a third time when bootstrapping just to work around this), I hope no one objects to a few small patches in the DMD code.

WalterBright · 2015-09-13T01:21:05Z

In that case, I'd like to see the workaround wrapped in a version(all) { workaround } else { original } along with a comment referencing the bugzilla issue and the DMD version where the bug is fixed.

CyberShadow · 2015-09-13T01:25:23Z

OK, will do. This pull request is still in early stages though (right now I want to use the autotester to check if a condition is ever triggered).

Off the top of your head, do you know if calling Outbuffer::setsize (from backend/outbuf.c) with a size that exceeds the buffer capacity is a valid operation?

WalterBright · 2015-09-13T01:34:30Z

Since it is implemented:

void Outbuffer::setsize(size_t size)
{
    p = buf + size;
}

I don't think so. Use Outbuffer::enlarge

CyberShadow · 2015-09-13T03:54:30Z

I'm asking because some DMD code is doing it. I added an assert, and the test suite passes with the assert, so it has to be a bug.

It looks like addtofixlist it passing an out-of-bounds offset to Obj::bytes: https://issues.dlang.org/show_bug.cgi?id=15019#c13

CyberShadow · 2015-09-13T14:30:21Z

I've cleaned up this PR and changed the Valgrind workarounds accordingly. It should be ready to review/merge. This doesn't fix issue 15019, but the changes here helped make the failure deterministic and reduce the test case, and are useful on their own.

yebblies · 2015-09-14T04:07:02Z

src/optimize.d

-                e.e2 = new IntegerExp(e.loc, e.e2.toInteger(), Type.tint64);
+            if (e.e2.op == TOKfloat64)
+            {
+                version (all)


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

Ping on this?

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.

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.

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

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

CyberShadow · 2015-09-14T17:49:24Z

grep: ./obj/freebsd/32/mytrace.log: No such file or directory
grep: ./obj/linux/32/mytrace.log: No such file or directory

Does anyone know what's with these test failures?

CyberShadow · 2015-09-14T17:50:00Z

Removed CyberShadow@d537ec8 because it is superseded by #5074

WalterBright · 2015-09-21T22:16:43Z

src/backend/machobj.c

@@ -2310,6 +2310,7 @@ void MachObj::addrel(int seg, targ_size_t offset, symbol *targsym,
        unsigned targseg, int rtype, int val)
 {
    Relocation rel;
+    memset(&rel, 0, sizeof(rel));


All the fields of rel are assigned values here. Adding a memset() is completely redundant, and makes it slower.

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.

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.

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));

SDrel is being used as a variable length array.

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.

It's still reasonable to memset in debug mode.

The right fix is to reorganize the type as I suggested.

CyberShadow · 2015-09-22T07:34:18Z

Wrapped the memset in #ifdef DEBUG.

yebblies · 2015-09-22T13:21:26Z

Auto-merge toggled on

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

Temtaime · 2015-09-23T22:18:57Z

src/backend/machobj.c

@@ -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));


It can be Relocation rel = {};

CyberShadow force-pushed the valgrind-20150912 branch 2 times, most recently from 8d2493a to 5d4ee37 Compare September 13, 2015 14:05

CyberShadow changed the title ~~[WIP] Valgrind fixes~~ Minor memory-safety fixes (Valgrind-identified problems, debug bounds checks) Sep 13, 2015

CyberShadow force-pushed the valgrind-20150912 branch from 5d4ee37 to d537ec8 Compare September 13, 2015 14:07

yebblies reviewed Sep 14, 2015
View reviewed changes

CyberShadow force-pushed the valgrind-20150912 branch from d537ec8 to 381e65f Compare September 14, 2015 17:49

WalterBright reviewed Sep 21, 2015
View reviewed changes

CyberShadow force-pushed the valgrind-20150912 branch from 381e65f to 097cc23 Compare September 22, 2015 07:34

CyberShadow added 3 commits September 22, 2015 12:56

Work around Valgrind choking on redundant REX.W prefix

4bc2030

Fix potential memory issues identified by Valgrind

8735fb8

hdrgen: Use OutBuffer.extractData()

d7091d5

CyberShadow force-pushed the valgrind-20150912 branch from 097cc23 to d7091d5 Compare September 22, 2015 12:56

yebblies added a commit that referenced this pull request Sep 22, 2015

Merge pull request #5068 from CyberShadow/valgrind-20150912

0284ca0

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

yebblies merged commit 0284ca0 into dlang:master Sep 22, 2015

Temtaime reviewed Sep 23, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

CyberShadow commented Sep 13, 2015

WalterBright commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

WalterBright commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

WalterBright commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

yebblies Sep 14, 2015

yebblies Sep 22, 2015

CyberShadow Sep 22, 2015

yebblies Sep 22, 2015

CyberShadow Sep 22, 2015

yebblies Sep 22, 2015

CyberShadow commented Sep 14, 2015

CyberShadow commented Sep 14, 2015

WalterBright Sep 21, 2015

CyberShadow Sep 21, 2015

WalterBright Sep 22, 2015

CyberShadow Sep 22, 2015

WalterBright Sep 22, 2015

CyberShadow Sep 22, 2015

yebblies Sep 22, 2015

WalterBright Sep 23, 2015

CyberShadow commented Sep 22, 2015

yebblies commented Sep 22, 2015

Temtaime Sep 23, 2015

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

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

Conversation

CyberShadow commented Sep 13, 2015

WalterBright commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

WalterBright commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

WalterBright commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

CyberShadow commented Sep 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyberShadow commented Sep 14, 2015

CyberShadow commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyberShadow commented Sep 22, 2015

yebblies commented Sep 22, 2015

Choose a reason for hiding this comment