重构嵌套循环后的段错误

Posted 2023-02-14

【中文标题】重构嵌套循环后的段错误

【英文标题】：Segfault after refactoring nested loops

【发布时间】：2021-12-30 10:49:25

【问题描述】：

我有一些来自我已经移植到 C 的数字音频课程的 MATLAB 代码。给定一个数字数据数组（例如，编码为双精度浮点数的 PCM 音频），生成一个数据段数组一个指定的宽度，并以指定的量相互重叠。这是相关代码。

typedef struct AudioFramesDouble 
    const size_t n,   // number of elements in each frame
                 num_frames;
    double* frames[];
 AudioFramesDouble;

/*
 * Produce a doubly-indexed array of overlapping substrings (a.k.a windows, frames,
 * segments ...) from a given array of data.
 *
 * x: array of (i.e., pointer to) data
 * sz: number of data elements to consider
 * n: number of elements in each frame
 * overlap: each frame overlaps the next by a factor of 1 - 1/overlap.
 */
AudioFramesDouble* audio_frames_double(register const double x[], const size_t sz, const unsigned n, const unsigned overlap) 
    // Graceful exit on nullptr
    if (!x) return (void*) x;
    const double hop_d = ((double) n) / ((double) overlap); // Lets us "hop" to the start of the next frame.
    const unsigned hop = (unsigned) ceil(hop_d);
    const unsigned remainder = (unsigned) sz % hop;
    const double num_frames_d = ((double) sz) / hop_d;
    const size_t num_frames = (size_t) (remainder == 0
                            ? floor(num_frames_d) // paranoia about floating point errors
                            : ceil(num_frames_d)); // room for zero-padding
    const size_t total_samples = (size_t) n * num_frames;

    AudioFramesDouble af = .n = n, .num_frames = num_frames;
    // We want afp->frames to appear as (double*)[num_frames].
    AudioFramesDouble* afp = malloc((sizeof *afp) + (sizeof (double*) * num_frames));
    if (!afp) return afp;
    memcpy(afp, &af, sizeof af);

    for (size_t i = 0; i < num_frames; ++i) 
        /* Allocate zero-initialized space at the start of each frame. If this
           fails, free up the memory and vomit a null pointer. */
        afp->frames[i] = calloc(n, sizeof(double));
        if (!afp->frames[i]) 
            double* p = afp->frames[i];
            for (long ii = ((long)i) - 1; 0 <= ii; ii--) 
                free(afp->frames[--i]);
            
            free(afp);
            return (void*) p;
        

        for (size_t j = 0, k; j < n; ++j) 
            if (sz <= (k = (i*hop) + j)) break;
            afp->frames[i][j] = x[k];
        
    

    return afp;

这按预期执行。我想将嵌套的 FOR 优化为以下内容

    for (size_t i = 0, j = 0, k; i < num_frames; (j == n - 1) ? (j = 0,i++) : ++j) 
    // If we've reached the end of the frame, reset j to zero.
    // Then allocate the next frame and check for null.
        if (j == 0 && !!(afp->frames[i] = calloc(n, sizeof(double)))) 
            double* p = afp->frames[i];
            for (long ii = ((long)i) - 1; 0 <= ii; ii--) 
                free(afp->frames[--i]);
            
            free(afp);
            return (void*) p;
        

        if (sz <= (k = (i*hop) + j)) break;
        afp->frames[i][j] = x[k];
    

这实际上编译并运行得很好；但在我的测试中，当我尝试访问最后一帧时，xFrames->frames[xFrames->num_frames-1], 我得到一个分段错误。这里发生了什么？我是否忽略了循环中的边缘情况？我一直在查看代码一段时间，但我可能需要第二双眼睛。对不起，如果答案很明显；我有点 C 新手。

附：我是无分支编程的粉丝，所以如果有人有关于删除这些 IF 的提示，我会全神贯注。我以前使用三元运算符，但为了调试的可读性而恢复为 IF。

【问题讨论】：

在风格上：return (void*) p; 相当糟糕！如果你想返回一个空指针，那么return NULL; 会好很多。尝试使用错误类型返回空指针，将其强制转换为泛型指针（用于编译器生成的到 AudioFramesDouble * 的隐式转换），这是相当糟糕的。

【参考方案1】：

请记住，逻辑运算符&& 和|| 执行short-circuit evaluation。

这意味着如果j != 0，那么您实际上不会调用calloc，并且您将在afp->frames[i] 中有一个无效的指针。

【讨论】：

这实际上是目标，否则几乎每次迭代后第 i 个指针都会被重置。无论何时j == 0，我们要么有i == 0，要么我们只是增加了i，表示下一帧的开始。

@terrygarcia 您是否尝试过使用调试器单步执行分配代码以查看它是否真的按预期工作？

@terrygarcia 整个代码看起来也相当复杂，因为它看起来可能是一个调用malloc 和memcpy 的简单循环。除了奇怪的回报，为什么要向下循环释放？没有必要这样做。在 free 循环中使用类似（因此令人困惑）变量 i 和 ii 保存。不要让代码变得过于复杂，从编写简单、干净和可读的代码开始。这也将使其在未来更易于维护。

我在想，如果i != 0 时某些分配失败，则有必要释放任何先前分配的内存。我转换为long 以允许使用负值（ii 读取为'整数i'）来打破循环，但单行if 也足够了。我想我也可以向上迭代。总而言之，我想创建一个几乎没有分支或嵌套的高效循环，这无疑为混淆打开了大门。尽管如此，我仍然不知道在最后一次 n 迭代中出了什么问题，尽管我猜最后一次 calloc 没有被调用。