Page MenuHomePhabricator

Fix for #pragma warning to work correctly with "1-4:" specifiers
ClosedPublic

Authored by andreybokhanko on May 19 2015, 9:06 AM.

Details

Summary

Bug fix for PR23577 (https://llvm.org/bugs/show_bug.cgi?id=23577#c0).

"1-4" specifiers are returned as numeric constants, not identifiers, and should be treated as such. Currently pragma handler incorrectly assumes that they are returned as identifiers.

Diff Detail

Repository
rL LLVM

Event Timeline

andreybokhanko retitled this revision from to Fix for #pragma warning to work correctly with "1-4:" specifiers.
andreybokhanko updated this object.
andreybokhanko edited the test plan for this revision. (Show Details)
andreybokhanko added reviewers: rnk, rsmith.
andreybokhanko added a subscriber: Unknown Object (MLST).
rnk edited edge metadata.May 19 2015, 9:25 AM

Can you add a -E test that verifies that these pragmas roundtrip through the pre-processor?

lib/Lex/Pragma.cpp
1075 ↗(On Diff #26067)

Make sure the SmallString has the same lifetime as Specifier.

1091 ↗(On Diff #26067)

utostr returns a std::string, so this would be a use-after-free. Use getSpelling() to get a StringRef that points into the original source code so we don't need memory allocation. You'll need to supply a SmallString, but 99% of the time it'll be unused.

andreybokhanko edited edge metadata.

Reid, thank you for the review!

I fixed the use-after-free bug you found (good catch, BTW!) I also created a test that checks -E (not sure this is exactly what you wanted; let me know if I misunderstood you).

rnk added inline comments.May 20 2015, 9:26 AM
test/Preprocessor/pragma_microsoft_E.c
1–3 ↗(On Diff #26147)

Rather than copy-pasting the file, you can actually have multiple RUN lines in a single test. So back in pragma_microsoft.c you can add the same RUN line you have here and the CHECK lines and it'll all work.

83 ↗(On Diff #26147)

You should check that the warning pragmas roundtrip through the preprocessor, though.

Test updated according to Reid's comments.

andreybokhanko added inline comments.May 21 2015, 5:57 AM
test/Preprocessor/pragma_microsoft_E.c
83 ↗(On Diff #26147)

Reid, sorry for a noob question -- what do you mean by checking "roundtrip through the preprocessor"? Is there an existing test I can use as an example?

In the updated patch I added CHECKs for preprocessor's output -- is this what you meant?

rnk added a comment.May 21 2015, 8:35 AM

Yes, check lines for pragma warning 1.

Sent from phone

Updated patch has these checks:

CHECK: #pragma warning(pop)
#pragma warning(1: 123)
CHECK: #pragma warning(1: 123)
#pragma warning(2: 234 567)
CHECK: #pragma warning(2: 234 567)
#pragma warning(3: 123; 4: 678)
CHECK: #pragma warning(3: 123)
CHECK: #pragma warning(4: 678)
#pragma warning(5: 123)
expected-warning {{expected 'push', 'pop', 'default', 'disable', 'error', 'once', 'suppress', 1, 2, 3, or 4}}

rnk accepted this revision.May 22 2015, 8:39 AM
rnk edited edge metadata.

lgtm, thanks!

This revision is now accepted and ready to land.May 22 2015, 8:39 AM

Reid, thank you for the quick and thoughtful review! This is my first ever clang fix and it is very nice to get such a good reception.

I don't have commit rights yet; Alexander [Musman], could you, please, commit the fix?

This revision was automatically updated to reflect the committed changes.