This is an archive of the discontinued LLVM Phabricator instance.

Change of UserLabelPrefix default value from "_" to ""
ClosedPublic

Authored by andreybokhanko on Jan 18 2016, 6:40 AM.

Details

Summary

@rafael proposed (http://reviews.llvm.org/D16138#325739) to change UserLablePrefix's default from "_" to "", as latter value happens much more often. This patch implements the proposal.

Yours,

Andrey Bokhanko

Software Engineer
Intel Compiler Team
Intel

Diff Detail

Repository
rL LLVM

Event Timeline

andreybokhanko retitled this revision from to Change of UserLabelPrefix default value from "_" to "".
andreybokhanko updated this object.
rafael edited edge metadata.Jan 18 2016, 7:00 AM

Thanks for working on this, but many of the changes setting the prefix to "_" look wrong.

lib/Basic/Targets.cpp
801 ↗(On Diff #45177)

This looks wrong, we produce a "f:" not an "_f:" when targeting powerpc-linux-gnu.

1617 ↗(On Diff #45177)

This also looks wrong.

3659 ↗(On Diff #45177)

This looks wrong.

3991 ↗(On Diff #45177)

This looks wrong.

4530 ↗(On Diff #45177)

This looks wrong.

5308 ↗(On Diff #45177)

This looks wrong.

5834 ↗(On Diff #45177)

This looks wrong.

6136 ↗(On Diff #45177)

This looks wrong.

6296 ↗(On Diff #45177)

This looks wrong.

6393 ↗(On Diff #45177)

This looks wrong.

6496 ↗(On Diff #45177)

This looks wrong.

@rafael, all these changes are driven by tests.

It seems you mean OS targeting, which is handled in other TargetInfo classes (LinuxTargetInfo in Linux case).

lib/Basic/Targets.cpp
801 ↗(On Diff #45177)

Is this commented out, tools/clang/test/Preprocessor/init.c:5216 fails. As can be seen in the test, PPC603E target expects UserLabelPrefix to be equal to "_" in freestanding mode.

As for powerpc-linux-gnu target, UserLabelPrefix is set to "" at lib/Basic/Target.cpp:416 (LinuxTargetInfo constructor).

1617 ↗(On Diff #45177)

Same as above -- NVPTX target expects UserLabelPrefix to be "_" in freestanding mode (tools/clang/test/Preprocessor/init.c:4853). Linux target is covered in LinuxTargetInfo constructor.

I am pretty sure the cases in init.c are wrong as the assembly itself
doesn't use a '_'.

Having said that, it is probably a good thing to do this in two steps.
So this patch LGTM on the condition that you also open a bug to audit
the cases where we define USER_LABEL_PREFIX to _ in init.c and CC
whoever added those.

Thanks,
Rafael

This revision was automatically updated to reflect the committed changes.

Rafael, thanks for the review!

I am pretty sure the cases in init.c are wrong as the assembly itself
doesn't use a '_'.

Having said that, it is probably a good thing to do this in two steps.
So this patch LGTM on the condition that you also open a bug to audit
the cases where we define USER_LABEL_PREFIX to _ in init.c and CC
whoever added those.

Done. https://llvm.org/bugs/show_bug.cgi?id=26255

Yours,
Andrey