This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Add LF_ prefix to LibFunc enums in TargetLibraryInfo. (NFC)
ClosedPublic

Authored by dlj on Jan 9 2017, 12:44 PM.

Details

Summary

The LibFunc::Func enum holds enumerators named for libc functions.
Unfortunately, there are real situations, including libc implementations, where
function names are actually macros (musl uses "#define fopen64 fopen", for
example; any other transitively visible macro would have similar effects).

Strictly speaking, a conforming C++ Standard Library should provide any such
macros as functions instead (via <cstdio>). However, there are some "library"
functions which are not part of the standard, and thus not subject to this
rule (fopen64, for example). So, in order to be both portable and consistent,
the enum should not use the bare function names.

The old enum naming used a namespace LibFunc and an enum Func, with bare
enumerators. This patch changes LibFunc to be an enum with enumerators prefixed
with "LF_". (Unfortunately, a scoped enum is not sufficient to override macros.)

There are additional changes required in clang.

Event Timeline

dlj updated this revision to Diff 83673.Jan 9 2017, 12:44 PM
dlj retitled this revision from to [Analysis] Add LF_ prefix to LibFunc enums in TargetLibraryInfo. (NFC).
dlj updated this object.
dlj added a reviewer: rsmith.
dlj added a subscriber: llvm-commits.

However, there are some "library"functions which are not part of the standard, and thus not subject to this rule (fopen64, for example). So, in order to be both portable and consistent, the enum should not use the bare function names.

Generally, our response to system headers which contain stupid #defines is just to #undef them. (See, for example, include/llvm/ADT/Triple.h.) Would that be practical in your situation?

dlj added a comment.Jan 20 2017, 3:04 PM

Not really... musl-libc is conforming, and the assumption that "memmove" is a valid token has different reasoning than that for "fopen64" (i.e., one is allowed by the C++ standard, the other is not).

(On a side note, I'm not sure #undef is a reasonable approach for any libc implementation, but in this particular case Clang is broken with a perfectly conformant libc. So that seems worthwhile to fix.)

dlj added a comment.Jan 20 2017, 3:08 PM

Actually, I spoke too soon: 17.6.4.3.1 specifically forbids #undef ing anything from a standard library header.

Appealing to the standard is not helpful here... fopen64 isn't defined by any standard, and therefore there isn't really any difference between using the token "fopen64" and the token "LF_fopen64". In practice, of course, the global namespace on most systems is polluted with a bunch of non-standard symbols, but we really don't want to rename stuff every time someone finds a system with an unusual #define, so therefore our usual solution is just to "#undef" the relevant symbol.

I guess you could argue that TargetLibraryInfo in particular is extremely likely to cause problems in the future, and therefore we should preemptively rename all the library functions to avoid problems.

dlj added a comment.Jan 20 2017, 5:23 PM

I guess you could argue that TargetLibraryInfo in particular is extremely likely to cause problems in the future, and therefore we should preemptively rename all the library functions to avoid problems.

Right... that's the impetus, so fopen64 may simply be a red herring. I called this out as "portability" in the summary, since the enumerators are names that could be macros on a conforming system. Do you have a specific suggestion for something that I could re-word?

rsmith edited edge metadata.Jan 23 2017, 11:16 AM

This makes sense to me; the current names seem pretty much optimally chosen to collide with C library functions (particularly ones that are not required to be #undef'd by a conforming implementation), as they are names we *know* that some C libraries provide as a non-conforming extension. Eli, can you make your peace with this, or do you still prefer that we #undef these names?

I guess it's okay.

I think I would prefer the prefix to spell out LibFunc; LF is a little confusing.

dlj updated this revision to Diff 85452.Jan 23 2017, 2:24 PM
  • Changed LF_ to LibFunc_.
dlj updated this revision to Diff 85456.Jan 23 2017, 2:32 PM
  • Re-flowed/tweaked some references that make the diff ugly.
efriedma accepted this revision.Jan 23 2017, 3:21 PM

LGTM with a couple minor tweaks. (If you don't have commit access, upload an updated version and ping me.)

lib/Transforms/Scalar/DeadStoreElimination.cpp
147

Not sure why you're changing this.

lib/Transforms/Scalar/MemCpyOptimizer.cpp
1277

?

This revision is now accepted and ready to land.Jan 23 2017, 3:21 PM
dlj marked an inline comment as done.Jan 23 2017, 3:25 PM
dlj added inline comments.
lib/Transforms/Scalar/DeadStoreElimination.cpp
147

Looks like a clang-tidy fix to me. (Un-)fixed.

dlj updated this revision to Diff 85470.Jan 23 2017, 3:26 PM
  • Address review comments.
This revision was automatically updated to reflect the committed changes.