Add -fno-merge-constants to default flags?

[chrisburr]

I've now had two different packages that broke when relocating due to -fmerge-constants being enabled by default (see the gitter link for an explanation of why this optimisation is problematic):

• https://gitter.im/conda-forge/conda-forge.github.io?at=61e18f469a3354540621b912
• https://github.com/mamba-org/mamba/issues/1517

@conda-forge/ctng-compiler-activation What do you think of adding -fno-merge-constants to the default flags?

[chrisburr]

@conda-forge/ctng-compiler-activation Any thoughts?

[beckermr]

cc @conda-forge/core @isuruf

I have no relevant expertise or opinions here myself.

[hmaarrfk]

This seems reasonable and necessary.

Conda assumes it can rewrite variables, but c is using them in a way that makes this impossible.

If you use conda's strategy of rewriting strings in code, you just tell the compiler not to reuse them.

As this is enabled at -O1 we should likely set this conda wide.

[beckermr]

For the uninitiated

-fmerge-constants
Attempt to merge identical constants (string constants and floating-point constants) across compilation units.

This option is the default for optimized compilation if the assembler and linker support it. Use -fno-merge-constants to inhibit this behavior.

Enabled at levels -O1, -O2, -O3, -Os.

[chrisburr]

Copying from gitter to try and explain exactly how this can go wrong:

• a C++ project with a variable like char* myvar = "... $PREFIX ... something";
• in another completely unrelated file there is MyFunc("something")
• compiler optimises something to only be written once in the binary
• MyFunc("something") is effectively called with a pointer to the same underlying data as myvar (i.e. MyFunc(myvar+N))
• when conda relocates the binary it pads myvar with null bytes causing the call to be changed to MyFunc("") (or MyFunc("hing") if the prefix is only slightly shorter)

I've only seen it happen on ppc64le, though I think it could happen for any software compiled with GCC (clang appears to not support this optimisation).

[hmaarrfk]

Thank you for copying that, hard to copy from a phone.

I'm not too familiar with this recipe but is the following file the best place to suggest an addition? https://github.com/conda-forge/ctng-compiler-activation-feedstock/blob/master/recipe/conda_build_config.yaml

It seems that some templating is being used to replace @CFLAGS@ in the activation scripts.

[beckermr]

Yes the flags go in there afaik.

[isuruf]

I don't think this is a good idea in general. Some packages might be relying on this behaviour to do quick string comparisons and we'll be breaking them.

[chrisburr]

I don't think this is a good idea in general. Some packages might be relying on this behaviour to do quick string comparisons and we'll be breaking them.

Can you think of any packages where this is actually the case? Unless it's widespread I think it's more important to avoid subtly broken packages which are hard to debug. I even could imagine this causing security issues if we get unlucky.

Even if someone has a package which is performing poorly when built for conda-forge, the compilation flags are an obvious place to check so hopefully it wouldn't be too hard to identify.

[isuruf]

I don't know of any package at the moment. I'm wary of adding flags that affect code generation and we've been trying to go the other way by removing default flags. (flags like -std=c++17 and -fopenmp are removed). This PR on the other hand goes the other way by adding a flag that affects code generation.

[chrisburr]

In general I agree with you but the need for this flag is really specific to how conda works and hard to identify when it goes wrong. Unlike the other flags you mention, it’s only an optimisation so it can’t cause builds to behave differently (ignoring a potential performance impact).

Ideally there would be a mechanism to prevent this optimisation for a specific string rather than all constants but I can’t find anything. If you want to be conservative we could consider only adding it for ppc64le as I’ve not yet seen an aarch64 or x86_64 build fail?

[isuruf]

Unlike the other flags you mention, it’s only an optimisation so it can’t cause builds to behave differently (ignoring a potential performance impact)

It can behave differently. That was my point above about string pointer comparisons being unequal with this flag set.

[chrisburr]

It can behave differently. That was my point above about string pointer comparisons being unequal with this flag set.

Sorry I struggle to see how code can rely on this as at -O0 this optimisation isn't applied meaning the code would only work with -O1 (or higher)?

There is also no guarantee that GCC will actually merge constants, even when passing -fmerge-all-constants.

[isuruf]

Maybe @katietz knows more about the consequences of enabling -fno-merge-constants by default?

[isuruf]

@katietz, said it's preferable to disabling the merge constants. i.e. adding -fno-merge-constants. @katietz, btw, -fmerge-constants is enabled at -O1, -O2, -O3, -Os.

[wolfv]

I think I cannot completely follow the first sentence. Did @katietz say that it is better to disable merge constants (in my interpretation that would be adding -fno-merge-constants)?

[chrisburr]

I also asked a few nearby compiler experts and they all think that:

• GCC makes no guarantees around this so it’s impossible to rely on constant merging
• if anything is affected by adding -fno-merge-constants it is already horribly broken

[beckermr]

Isuru's message has a typo @wolfv. You are correct that we should add -fno-merge-constants.